miner: always assume we can build witness blocks #24421
Conversation
glozow
force-pushed
the
2022-02-miner-fincludewitness
branch
from
5ead13f to
03c37cc
Compare
|
Concept and code review ACK 03c37cc |
|
Concept ACK |
1 similar comment
|
Concept ACK |
| self.skip_mine(self.nodes[2], wit_ids[NODE_2][P2WSH][0], True) # block 425 | ||
| self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][P2WPKH][0], True) # block 426 | ||
| self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][P2WSH][0], True) # block 427 | ||
| self.generate(self.nodes[0], 264) # block 427 |
There was a problem hiding this comment.
Not really a fan of removing this test. Why not keep it to make the change in behaviour explicit and clarify that mining before segwit is active will result in a json exception?
There was a problem hiding this comment.
I disagree. Why do we need to test that a miner on a pre-segwit chain with segwit transactions in its mempool fails to mine a block? If we think that this scenario is possible (and so needs testing), then we shouldn't make the change in this PR, since failing to mine a block is strictly worse behaviour than mining a block without the segwit transaction.
There was a problem hiding this comment.
I guess this way we have test coverage for "unexpected witness"
There was a problem hiding this comment.
I guess this way we have test coverage for "unexpected witness"
That's a very indirect way of testing a failure condition in ContextualCheckBlock(). If we want a test for that condition, then we should have a functional test where a non-witness block (which is possible to create even after segwit activation) is stuffed with witness data. That's a condition that could happen on mainnet, and so isn't relying on impossible-to-hit-in-production mining code. Even better would be a unit test for ContextualCheckBlock().
I think it's inconsistent to remove production code that is "impossible to hit" and then add a test that hits that exact condition.
There was a problem hiding this comment.
I see the point why this practically doesn't need test coverage. I also think the changes here are safe to make. However, I think we should still think about the case where our assumptions fail or our expectations aren't met. It is minimally useful for the test to remind reviewers what will happen in the unexpected case.
For example, I disagree about:
failing to mine a block is strictly worse behaviour than mining a block without the segwit transaction.
If this code path were to be triggerable by an attacker, it would be a strictly better outcome to fail to mine a block before witness than to mine a block. First, a block mined far behind the tip will never be accepted into the main chain and thus never financially rewarded, so there is no benefit of mining it. Second, returning an error early may tell the attacked miner that they have been eclipsed earlier. Third, mining a block (and submitting it, then later reorging it (if it was accepted)) is going to execute more code during the eclipse attack than not mining a block. If there are bugs in the executed code, it gives more tools to the attacker for no benefit. So I think it also serves as a belt-and-suspender.
Another example (unrelated to this pull request):
If an attacker could make the blocksdir unusable (until a manual -reindex) by executing a reorg that is large enough that we think it won't happen, I still think we should fix the bug so that the blocksdir remains usable after any states the software went through.
There was a problem hiding this comment.
Removing this test would also fix the "unexpected witness" CI failures
Given the low possibility of a reorg reverting the segwit soft fork, there is no need to check whether segwit is active here. Also, TestBlockValidity is run on the block template after it has been created.
glozow
force-pushed
the
2022-02-miner-fincludewitness
branch
from
03c37cc to
40e871d
Compare
|
Concept ACK |
|
ACK 40e871d |
|
(See also #19391 for more dead code) |
|
utACK 40e871d, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: #24421 (comment). |
There was a problem hiding this comment.
LGTM.
I think there is a small theoretical concern that this will make Bitcoin Core internally minimally more inconsistent. See the comment why I initially held back on proposing this pull.
It is safe to make the changes here, because we assume that the nMinimumChainWork bundled in any release that will include this patch is sufficiently large to make reorg-attacks during IBD while eclipsed prohibitively expensive. The same assumption was made in commit 0a8b7b4 and #23536
| self.skip_mine(self.nodes[2], wit_ids[NODE_2][P2WSH][0], True) # block 425 | ||
| self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][P2WPKH][0], True) # block 426 | ||
| self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][P2WSH][0], True) # block 427 | ||
| self.generate(self.nodes[0], 264) # block 427 |
There was a problem hiding this comment.
I see the point why this practically doesn't need test coverage. I also think the changes here are safe to make. However, I think we should still think about the case where our assumptions fail or our expectations aren't met. It is minimally useful for the test to remind reviewers what will happen in the unexpected case.
For example, I disagree about:
failing to mine a block is strictly worse behaviour than mining a block without the segwit transaction.
If this code path were to be triggerable by an attacker, it would be a strictly better outcome to fail to mine a block before witness than to mine a block. First, a block mined far behind the tip will never be accepted into the main chain and thus never financially rewarded, so there is no benefit of mining it. Second, returning an error early may tell the attacked miner that they have been eclipsed earlier. Third, mining a block (and submitting it, then later reorging it (if it was accepted)) is going to execute more code during the eclipse attack than not mining a block. If there are bugs in the executed code, it gives more tools to the attacker for no benefit. So I think it also serves as a belt-and-suspender.
Another example (unrelated to this pull request):
If an attacker could make the blocksdir unusable (until a manual -reindex) by executing a reorg that is large enough that we think it won't happen, I still think we should fix the bug so that the blocksdir remains usable after any states the software went through.
|
ACK 40e871d |
|
I'm sometimes seeing from feature_segwit.py, generated by that looks like it might be due to this PR? (Took about 80 runs to reproduce) |
|
Probably happens because the witness txs from node 2 are sent to node 0? You can check this by adding a |
|
The problem doesn't appear to get hit prior to this PR (after ~250 runs). Adding Seems to me that random test suite failures makes this worth reverting... |
That's my guess as well. Can probably fix by disconnecting node0/node2 before generate, and then reconnecting after 431 is mined. But definitely feel free to revert, sorry for missing this! |
|
I think there are two options:
|
In order to determine whether to download or process a relayed transaction, we try to determine if we already have the transaction, either in the mempool, in our recently rejected filter, in our orphan pool, or already confirmed in the chain itself. Prior to this commit, the heuristic for checking the chain is based on whether there's an output corresponding to the 0- or 1-index vout in our coin cache. While that is a quick check, it is very imprecise (say if those outputs were already spent in a block) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will capture the case of a transaction which has been confirmed and then fully spent already. To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
My vote would be for removing the invalid test :) |
|
##24578 ;) |
…e_segwit d6b24e1 test: Fix intermittent failure in feature_segwit (DrahtBot) Pull request description: There are intermittent failures on current master (bitcoin/bitcoin#24421 (comment)) as the witness txs occasionally make it into other node's mempools when they shouldn't. Fix this by removing the test as suggested earlier in bitcoin/bitcoin#24421 (comment). Fixes bitcoin/bitcoin#24590 ACKs for top commit: aureleoules: tACK d6b24e1. jnewbery: Code review ACK d6b24e1 Tree-SHA512: ef7be52bdbde97b6921b44824e7fbc2dd92b77b8f076794867259f2b0bff08e0def06f4b2576f63a8ccb737a15cd8441fb7db20fa1f0d4fa9c7c76d5b83388ef
d6b24e1 test: Fix intermittent failure in feature_segwit (DrahtBot) Pull request description: There are intermittent failures on current master (bitcoin#24421 (comment)) as the witness txs occasionally make it into other node's mempools when they shouldn't. Fix this by removing the test as suggested earlier in bitcoin#24421 (comment). Fixes bitcoin#24590 ACKs for top commit: aureleoules: tACK d6b24e1. jnewbery: Code review ACK d6b24e1 Tree-SHA512: ef7be52bdbde97b6921b44824e7fbc2dd92b77b8f076794867259f2b0bff08e0def06f4b2576f63a8ccb737a15cd8441fb7db20fa1f0d4fa9c7c76d5b83388ef
Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc).
TestBlockValidity()is also run on the block template at the end ofCreateNewBlock(), so any invalid block would be caught there.