This Commit does 3 things:
sign_tx() function1127 | @@ -1125,7 +1128,6 @@ def run_test(self): 1128 | tx1.vout.append(CTxOut(0, CScript([OP_TRUE]))) 1129 | tx1.vout.append(CTxOut(0, CScript([OP_TRUE]))) 1130 | tx1.vout.append(CTxOut(0, CScript([OP_TRUE]))) 1131 | - tx1.calc_sha256()
Any reason to remove these? It seems safer to keep them just so the cached hash is up to date. (Future changes are less likely to break due to this, I'd say)
The very next line of the code is sign_tx() which changes the signature data. And the line after it calls rehash() to forcibly calculate it again. So, this cached value was not being used. I don't mind adding it again
I also found 1 more comment inconsistent change which can be added with this PR. I will change those shortly.
EDIT: changed a comment in sign_tx() to match the code.
730 | @@ -725,7 +731,6 @@ def run_test(self): 731 | assert(len(out[17].vout) < 42) 732 | tx.vin.append(CTxIn(COutPoint(out[17].sha256, 42), CScript([OP_TRUE]), 0xffffffff)) 733 | tx.vout.append(CTxOut(0, b"")) 734 | - tx.calc_sha256()
At all these places, tx.calc_256() call was before update_block() which forcefully rehash the cached value.
Please change the first line of your commit to a better summary of the changes—This Commit does 3 things: is too general
Tools that process git history rely on the following commit message format:
<title>
<empty line>
<summary> (can be multiple lines)
...
Changed the commit message.
Also removed "clean up uses of rehash or calc_sha256 where it was not needed" part from the commit since it does not add much value and is difficult to review.
Weird, this is failing for resendwallettransactions.py . Seems like a issue in CI system.
<!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 424 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
641 | @@ -640,6 +642,13 @@ def run_test(self): 642 | self.sync_blocks([b55], True) 643 | self.save_spendable_output() 644 | 645 | + # The block which was previously rejected because of being "too far(3 hours)" must be accepted 2 hours later. That is 646 | + # the new block is only 1 hour into future now 647 | + self.log.info("Accept a previously rejected future block at a later time") 648 | + node.setmocktime(int(time.time()) + 2*60*60) 649 | + self.sync_blocks([b48], success=False) # The tip is not updated because b55 is seen first
TypeError: sync_blocks() got an unexpected keyword argument 'success'
Hi @sanket1729 - I'm getting the same error as Marco mentioned above. Is this something you plan on addressing?
Hi @adamjonas @MarcoFalke, sorry for being unresponsive. I don't remember why I had this PR in the first place, but it looks like there is some value in it.
I will fix the build within shortly.
I don't know the policy for commits in bitcoin, but for now, I have separated the comment code inconsistencies and the new test into separate commits. Let me know if you prefer having them in a single squashed commit.
Verified that this now passes the tests locally.
Let me know if you prefer having them in a single squashed commit.
I think it's reasonable to keep them separated given they could each be cherry-picked independently. If you are keen on reworking the commit messages though, the post linked from the docs does a good job explaining what messages are commonly considered good practice.
Thanks, I learned something new. I will follow the same practices for my other projects.
Edit: Up for review again, the build is passing, and commit messages are fixed according to the developer recommendations.
Approach ACK
Code review ACK 5f4e969a97880462e0296d59a8d8f01f92666c39 ACK 50055c468110796ca7cd1ff3cea7f486e035af47
I have tested the change locally.
Tested ACK https://github.com/bitcoin/bitcoin/commit/5f4e969a97880462e0296d59a8d8f01f92666c39 and https://github.com/bitcoin/bitcoin/commit/50055c468110796ca7cd1ff3cea7f486e035af47 on Ubuntu 20.04. I agree on the increase in the test coverage and on the adjustments in the code / comment.
Tested ACK
ACK https://github.com/bitcoin/bitcoin/pull/14604/commits/5f4e969a97880462e0296d59a8d8f01f92666c39
feature_test.py which doesn't exist)
Thanks for the review everyone :). I have changed the typo commit message as pointed out by Nelson Galdeman.
Hi @sanket1729 - I would like to ask you a question to better understand or in case I am missing something: when creating the new tx in b44, I couldn’t find a corresponding fee explicitly added to the coinbase value. I just wanted to ask you if I am correct and, in that case, if this was intended and it´s OK for this test case.
Hi @simsbluebox, yes the tx fees are not added to the coinbase output. The consensus rule only enforces that that coinbase_output <= block_reward + fees.
You are right that the output could have included the fees in it too, but it is unnecessary for this test. Adding the additional fees would (slightly)complicate the code, and testing fee reward calculation is not the subject of this particular test.
Thank you for your answer, @sanket1729 !
ACK https://github.com/bitcoin/bitcoin/pull/14604/commits/ad37c0b254e8ec987bc42dce7de3fc25d1f7cc03 + https://github.com/bitcoin/bitcoin/pull/14604/commits/ce656e8a55b684430f9270cdce6dcfe379cf6ab9, I built & ran tests on Debian 10.8 (buster) and reviewed the code and it looks OK for me.
Off-PR note: In my environment, the test "re-org of one week's worth of blocks (1088 blocks)" required more than 2 GB RAM to go on successfully.
Tested and code review ACK ce656e8
Also just a nit with the title. I don't think the refactor part is necessary as it seems quite minimal. Maybe it's best we mention the test added like test: Add accept rejected future block at later time :)
699 | @@ -698,6 +700,13 @@ def run_test(self): 700 | self.send_blocks([b55], True) 701 | self.save_spendable_output() 702 | 703 | + # The block which was previously rejected because of being "too far(3 hours)" must be accepted 2 hours later. 704 | + # The new block is only 1 hour into future now 705 | + self.log.info("Accept a previously rejected future block at a later time") 706 | + node.setmocktime(int(time.time()) + 2*60*60) 707 | + self.send_blocks([b48], success=False) # The tip is not updated because b55 is seen first
Not sure how useful it is to test with success=False
Maybe add a block and invalidate it after success?
success=False would still error if given an invalid block. It only marks whether the tip was updated.
It is however still a good addition to the test to actually show the reorg. Updated the test and rebased to the latest master to avoid long-range merges
ACK. Can be merged, but improvements are also possible
1) Makes the code for block 44 consistent with the expected figure in
the comment above it by adding a transaction to the block
2) Fixed comment describing sign_tx() function
Adds a test case for checking reacceptance a previously rejected block
that was too far in the future.
reACK 5531119
reACK 55311197c483477b79883da5da09f2bc71acc7cf
Ran feature_block.py several times on MacOS 11.3, passed.