rpc: Remove submitblock pre-checks

TheCharlatan commented at 11:05 am on October 29, 2024: contributor

With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden.

The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.

The coinbase check is repeated again early during ProcessNewBlock. Pre-checking it may also shadow more fundamental problems with a block. In most cases the block header is checked first, before validating the transactions. Checking the coinbase first therefore masks potential issues with the header. Fix this by removing the pre-check.

Similary the duplicate checks are repeated early in the contextual checks of ProcessNewBlock. If duplicate blocks are detected much of their validation is skipped. Depending on the constitution of the block, validating the merkle root of the block is part of the more intensive workload when validating a block. This could be an argument for moving the pre-checks into block processing. In net_processing this would have a smaller effect however, since the block mutation check, which also validates the merkle root, is done before.

Testing spamming a node with valid, but duplicate unrequested blocks seems to exhaust a CPU thread, but does not seem to significantly impact keeping up with the tip. The benefits of adding these checks to net_processing are questionable, especially since there are other ways to trigger the more CPU-intensive checks without submitting a duplicate block. Since these DOS concerns apply even less to the RPC interface, which does not have banning mechanics built in, remove them too.

Finally, also remove the pre-checks from bitcoin-chainstate.cpp.

This PR is part of the libbitcoinkernel project.

DrahtBot commented at 11:05 am on October 29, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31175.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on Oct 29, 2024

mzumsande commented at 3:22 pm on October 29, 2024: contributor

Haven’t looked deeply yet, just wanted to mention #10146, which introduced the coinbase check and was even backported (see also #10190 for a regression test). Makes me wonder if there is more historical context to this.

TheCharlatan commented at 10:11 am on October 31, 2024: contributor

Makes me wonder if there is more historical context to this.

Yes, me too. If there are good reasons to keep these checks, then we should document why (and why they are not needed for the other interfaces). I could not come up with one, so I opened this PR, but I may well be missing something.

instagibbs commented at 8:50 pm on November 1, 2024: member

my guess is from #10146 this commit ada0caa165905b50db351a56ec124518c922085a looks like a covert fix for submitblock with a completely empty block. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mining.cpp#L1041 shows that it will call:

UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex which would result in UB, before any PoW checks are made.

mzumsande commented at 9:00 pm on November 6, 2024: contributor

Concept ACK

I agree with the general motivation. While this PR can change the error message of submitblock in case more than one thing is wrong with the block, I don’t really see why one is more important than the other.

UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex which would result in UB, before any PoW checks are made.

Nice find! I think that it would be good to have a test that would trigger the UB if https://github.com/bitcoin/bitcoin/commit/ada0caa165905b50db351a56ec124518c922085a was reverted (it requires us to know the header so that UpdateUncommittedBlockStructures() gets called, so the added test for an empty block doesn’t trigger it - could make another deepcopy of block instead and then delete the txns):

Fix this by removing the pre-check, but still throwing the same more expressive exception in case a coinbase validation error occurs.

I realize that you want to keep the existing behavior in this PR, but I do wonder if it really makes sense to have other consensus errors return “rejected”, and this particular one to throw an exception.

TheCharlatan force-pushed on Nov 7, 2024

TheCharlatan commented at 9:18 pm on November 7, 2024: contributor

Updated e742d66b1c77a90de502c45b452184b183b3778c -> b83d509d59dde352054cccf792d28bb87835e64f (submitblock_prechecks_0 -> submitblock_prechecks_1, compare)

Addressed @mzumsande’s comment, added a small addition to the functional test for checking blocks with no transactions.
Mentioned the commit introducing the patch to GetWitnessCommitmentIndex in the commit message.

I realize that you want to keep the existing behavior in this PR, but I do wonder if it really makes sense to have other consensus errors return “rejected”, and this particular one to throw an exception.

Yeah, I also don’t think it makes a lot of sense, and even though it looks like the original description in #10146 was used to mask an actual problem, it does still mention the distinct error message as an improvement, so not sure if removing it could be annoying for users.

in test/functional/mining_basic.py:251 in eedbce9dea outdated

247+        bad_block.solve()
248+        assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, bad_block.serialize().hex())
249+        no_tx_block = copy.deepcopy(block)
250+        no_tx_block.vtx.clear()
251+        no_tx_block.solve()
252         assert_raises_rpc_error(-22, "Block does not start with a coinbase", node.submitblock, bad_block.serialize().hex())

mzumsande commented at 9:35 pm on November 7, 2024:

submit no_tx_block instead of bad_block here, and if I remember correctly from yesterday, that will lead to a different error.

TheCharlatan commented at 10:35 pm on November 7, 2024:

Sorry, fixed. Makes me even less certain on keeping the coinbase error around. I’ll sleep on it.

TheCharlatan force-pushed on Nov 7, 2024

DrahtBot added the label CI failed on Nov 7, 2024

rpc: Remove submitblock pre-checks

With the introduction of a mining ipc interface and the potential future
introduction of a kernel library API it becomes increasingly important
to offer common behaviour between them. An example of this is
ProcessNewBlock, which is used by ipc, rpc, net_processing and
(potentially) the kernel library. Having divergent behaviour on
suggested pre-checks and checks for these functions is confusing to both
developers and users and is a maintenance burden.

The rpc interface for ProcessNewBlock (submitblock) currently pre-checks
if the block has a coinbase transaction and whether it has been
processed before. While the current example binary for how to use the
kernel library, bitcoin-chainstate, imitates these checks, the other
interfaces do not.

The coinbase check is repeated again early during ProcessNewBlock.
Pre-checking it may also shadow more fundamental problems with a block.
In most cases the block header is checked first, before validating the
transactions. Checking the coinbase first therefore masks potential
issues with the header. Fix this by removing the pre-check.

The pre-check was likely introduced on top of
ada0caa165905b50db351a56ec124518c922085a to fix UB in
GetWitnessCommitmentIndex in case a block's transactions are empty. This
code path could only be reached because of the call to
UpdateUncommittedBlockStructures in submitblock, but cannot be reached
through net_processing.

Similary the duplicate checks are repeated early in the contextual
checks of ProcessNewBlock. If duplicate blocks are detected much of
their validation is skipped. Depending on the constitution of the block,
validating the merkle root of the block is part of the more intensive
workload when validating a block. This could be an argument for moving
the pre-checks into block processing. In net_processing this would have
a smaller effect however, since the block mutation check, which also
validates the merkle root, is done before.

Testing spamming a node with valid, but duplicate unrequested blocks
seems to exhaust a CPU thread, but does not seem to significantly impact
keeping up with the tip. The benefits of adding these checks to
net_processing are questionable, especially since there are other ways
to trigger the more CPU-intensive checks without submitting a duplicate
block. Since these DOS concerns apply even less to the RPC interface,
which does not have banning mechanics built in, remove them too.

ab3a67b600

kernel: Make bitcoin-chainstate's block validation mirror submitblock's

The behaviour of submitblock was changed in the previous commit, so
change it here too.

309bd56d97

TheCharlatan force-pushed on Nov 8, 2024

TheCharlatan commented at 9:30 am on November 8, 2024: contributor

Updated b83d509d59dde352054cccf792d28bb87835e64f -> 309bd56d97f87f973f45897fc00b1bd2fc5cff1a (submitblock_prechecks_1 -> submitblock_prechecks_2, compare)

Removed throw on missing coinbase in submitblock.

DrahtBot removed the label CI failed on Nov 8, 2024

in src/rpc/mining.cpp:1029 in ab3a67b600 outdated

1024-    {
1025-        LOCK(cs_main);
1026-        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
1027-        if (pindex) {
1028-            if (pindex->IsValid(BLOCK_VALID_SCRIPTS)) {
1029-                return "duplicate";

mzumsande commented at 7:31 pm on November 18, 2024:

I think that this will change behavior in the case where we re-submit a previously pruned block with submitblock:

On master, we’d skip early here and return “duplicate” because the index is still BLOCK_VALID_SCRIPTS-valid. But with his branch, we’d accept the block and save it to disk once more.

I’m really unsure if that is an undesired change in behavior or not - I guess it could also be viewed as a feature. In any case, it’s a behavior change that should be documented if we do go through with it.

TheCharlatan commented at 9:08 pm on November 18, 2024:

Good catch, thank you for taking another look! I’m also not sure what to do in light of this. At the minimum this reason for the duplicate check should be documented. I feel like with this kind of change I would err on the conservative side and not introduce potentially breaking behaviour. Even if this might be a nice way to inject block data into the node, I don’t know if this is something anybody needs, even if it now more closely mimics getblockfrompeer.

We could keep the duplicate check and remove the invalid duplicate check, but I feel like requiring a pre-check to handle the pruning and force processing case is not very elegant. At the same time we can’t move the check into ProcessNewBlock, since it would break getblockfrompeer as far as I can tell. What do you think?

bitcoin-chainstate currently does not support pruning (though it also sets force processing), so removing it there should not have the same effect.

mzumsande commented at 6:56 pm on November 20, 2024:

err on the conservative side and not introduce potentially breaking behaviour

Generally speaking, I don’t see reason why we should artificially restrict a user from submitting a previously block via RPC, when they could request that block via getblockfrompeer, and I can’t of use cases it would break.

Maybe one way to move forward would be to not remove the duplicate check in this PR (but document why it exists) - and instead remove it in a follow-up PR, together with a release note about the changed behavior. @Sjors: FYI, since you introduced getblockfrompeer, do you remember any discussions about also allowing to re-submit previously pruned blocks via RPC / do you see a problem with that?

instagibbs commented at 6:59 pm on November 20, 2024:

The only danger I can foresee is if someone was somehow relying on them not being able to be committed to disk again. Would there be any performance concerns with commiting+re-pruning or similar?

rpc: Remove submitblock pre-checks #31175

Code Coverage & Benchmarks

Reviews

Conflicts