New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only call NotifyBlockTip when chainActive changes #12431
Only call NotifyBlockTip when chainActive changes #12431
Conversation
0eb52a0
to
8354565
Compare
test/functional/rpc_blockchain.py
Outdated
|
||
assert_waitforheight(0) | ||
assert_waitforheight(current_height - 3) | ||
assert_waitforheight(current_height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also want to add a timeout case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Played around with doing this, but found that the return from RPC is (unfortunately) no different in the timeout case, meaning we'd have to spin up a separate thread in-test, run a timer, and then assert that the RPC thread hadn't yet stopped execution -- probably overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a testcase for current_height + 1
, though.
Demonstrates the presence of a bug in in `validation.cpp:InvalidateBlock` which will update `rpc/blockchain.cpp:latestblock` erroneously.
Previously, if `invalidateblock` was called on a block in a branch, NotifyBlockTip would be called on that block's predecessor, creating an incorrect `rpc/blockchain.cpp:latestblock` value. Only call NotifyBlockTip if the chain being modified is activeChain.
8354565
to
f98b543
Compare
Thanks for the look, @conscott. Rebased. |
utACK f98b543 |
@jamesob, I'm curious how this might interact with #11856 with gets rid of the uiInterface.NotifyBlockTip signal, replacing it with CValidationInterface::UpdatedBlockTip listeners. EDIT: Looking into this more, this bugfix makes sense in combination with #11856, and would prevent spurious UpdatedBlockTip notifications in that PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK f98b543. This fixes a pretty clear waitforblockheight
bug and adds a nice test case.
utACK f98b543 |
@TheBlueMatt, this is a simple, one line change fixing the |
Without thinking about it too hard, this seems fine. I'd obviously like to move towards getting rid of uiInterface calls in validation, ala #11856, but this should likely be applied there as well. |
@laanwj, I think this looks good to merge |
f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial #12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f
f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f
f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f
f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f
f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f
f98b543 Only call NotifyBlockTip when the active chain changes (James O'Beirne) 152b7fb [tests] Add a (failing) test for waitforblockheight (James O'Beirne) Pull request description: This is a subset of the more controversial bitcoin#12407, but this also adds a test demonstrating the bug. In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up `waitforblockheight` (or anything else relying on `rpc/blockchain.cpp:latestblock`) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC. Only call NotifyBlockTip when the block being marked invalid is on the active chain. Tree-SHA512: 9a54fe5e8c7eb489daf5df4483c0986129e871e2ca931a456ba869ecb5d5a8d4f7bd27ccc9e711e9292c9ed79ddef896c85d0e81fc76883503e327995b0e914f
This is a subset of the more controversial #12407, but this also adds a test demonstrating the bug.
In InvalidateBlock, we're calling NotifyBlockTip with the now-invalid block's prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up
waitforblockheight
(or anything else relying onrpc/blockchain.cpp:latestblock
) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC.Only call NotifyBlockTip when the block being marked invalid is on the active chain.