Fix transaction size comments. Size now refers to virtual size (BIP141). #1
Conversation
|
I've built and tested the changes. |
sdaftuar
left a comment
sdaftuar
left a comment
There was a problem hiding this comment.
One nit, but otherwise looks good to me.
As a matter of good practice, in addition to building your changes locally, you should also locally run the tests (make check + qa/pull-tester/rpc-tests.py) to make sure nothing inadvertently broke.
src/rpc/mining.cpp
Outdated
There was a problem hiding this comment.
nit: I might drop the word virtual here? I think the last sentence explains sufficiently...
src/rpc/blockchain.cpp
Outdated
There was a problem hiding this comment.
This doesnt elucidate what this is to me - maybe the second sentence should be like "Differs from size for witness transactions as this does not include the witness data" or something like that (I dont actually know what its supposed to be).
There was a problem hiding this comment.
His language is very similar to the explanatory text for "vsize" used in the rawtransaction rpc's. Unfortunately, BIP 141 doesn't define virtual transaction size, but I think the best way to wrap this up would be to use the term virtual size here, and then suggest that they update the BIP to define it as well.
Here, virtual size refers to 1/4 of the transaction weight (so base_tx_bytes + witness_size/4).
There was a problem hiding this comment.
@sdaftuar I've opened a PR against the BIPS repository: ChaincodeResidency/bips#1
There was a problem hiding this comment.
Seems reasonable, though you might have write "See BIP 141" instead of just "BIP 141" :).
There was a problem hiding this comment.
Sure thing. Please see 73b772d
sdaftuar
left a comment
sdaftuar
left a comment
There was a problem hiding this comment.
Looks good to me. Just make sure you squash these down to one commit before opening the PR upstream.
Since you reference definitions in the BIP, you may want to open the BIP PR first (I'll review that now) and then in the PR text for this pull, link to your BIP 141 update PR as well.
jnewbery
force-pushed
the
rpc_comments
branch
from
73b772d to
2ff29a7
Compare
|
I've opened a pull request to bitcoin/bitcoin. Closing this PR. |
…ter-return checking 8d22ab0 ci: Enable address sanitizer (ASan) stack-use-after-return checking (practicalswift) Pull request description: Enable address sanitizer (ASan) stack-use-after-return checking (`detect_stack_use_after_return=1`). Example: ``` #include <iostream> #include <string> const std::string& get_string(int i) { return std::to_string(i); } int main() { std::cout << get_string(41) << "\n"; } ``` Without address sanitizer (ASan) stack-use-after-return checking: ``` $ ./stack-use-after-return $ ``` With address sanitizer (ASan) stack-use-after-return checking: ``` $ ASAN_OPTIONS="detect_stack_use_after_return=1" ./stack-use-after-return ================================================================= ==10400==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f7fa0400030 at pc 0x00000049d2cc bp 0x7ffcbd617070 sp 0x7ffcbd616820 READ of size 2 at 0x7f7abbecd030 thread T0 #0 0x439781 in fwrite #1 0x7f7ac0504cb3 in std::basic_ostream<char, std::char_traits<char> >& std::__ostream_insert<char, std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*, long) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0x113cb3) #2 0x4f9b5f in main stack-use-after-return.cpp:9:15 #3 0x7f7abf440b96 in __libc_start_main #4 0x41bbc9 in _start … $ ``` Top commit has no ACKs. Tree-SHA512: 6557a9ff184023380fd9aa433cdf413e01a928ea99dbc59ec138e5d69cb9e13592e8bb5951612f231ff17a37a895bec5c0940c8db5f328a5c840a5771bdeeba5
3 participants
As part of BIP 141, transaction sizes have been replaced by virtual size (with witness data discounted). Code comments and rpc help text have not been updated everywhere to reflect this change.
This PR fixes various comments and rpc help text.