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
ACK	Sjors, instagibbs, mzumsande, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31384 (mining: bugfix: Fix duplicate coinbase tx weight reservation by ismaelsadeeq)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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

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 pruned 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?

TheCharlatan commented at 12:32 pm on November 21, 2024:

I decided to add a release note for now. I think the changes in this PR are small enough that we can hash the changes out wholesale and it is also not a particularly pressing change. Still not wholly convinced, but thinking about it some more I don’t think somebody was relying on this behaviour beforehand either.

Sjors commented at 2:45 pm on November 21, 2024:

@mzumsande I don’t recall such discussions. The only thing that matters for getblockfrompeer is that it can store previously pruned blocks.

That said, I do think it’s useful to allow submitblock to store a previously pruned block. It lets a user dump the block on one node and submit it on this node, without having go through the effort of establishing a p2p connection, figuring out the peer id and then calling getblockfrompeer.

Sjors commented at 3:04 pm on November 21, 2024:

@maflcko it’s probably useful to briefly reopen #10146 and link to this thread for the likely real reason for that PR.

I’m speculating it was done in a stealthy way because there might be someone running a public service to submit blocks, and it would use that RPC under the hood. Or they were worried that since mining pools use this RPC internally (see e.g. #3658, #5538 (comment)) they might have it exposed to the internet - or at least blindly forward “blocks” to it. And therefore could be attacked.

cc @dergoegge @darosior in case you want to add it to the disclosure overview.

darosior commented at 7:02 pm on November 26, 2024:

I do not have the context nor the bug report behind this fix, but #10146 is clearly preventing a crash on accessing block->vtx[0] when an empty block is submitted through submitblock. In addition the introduction of the check for a valid coinbase transaction smells like preventing https://gnusha.org/url/?https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html but i can’t think of how it could be exploited.

Typically you would think of an attack along these lines:

Miner A is exposing submitblock.
Miner B mines a valid block and a malleated, invalid, block with the same hash.
Miner B sends the malleated block to Miner A’s submitblock API, which stores it as invalid.
Miner A broadcasts his valid block.
Miner B rejects it. REKT.

But submitblock uses ProcessNewBlock to store the block. And #10146 is after #9765, which makes sure CheckBlock is called (and therefore the coinbase check performed) before persisting the header. @gmaxwell was there more to #10146 than the crash fix?

TheCharlatan force-pushed on Nov 21, 2024

TheCharlatan commented at 12:27 pm on November 21, 2024: contributor

309bd56d97f87f973f45897fc00b1bd2fc5cff1a -> 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5 (submitblock_prechecks_2 -> submitblock_prechecks_3, compare)

Added release notes documenting the submitblock change w.rt. duplicate block handling and pruning.
Split the changes into more incremental commits.

Sjors commented at 3:26 pm on November 21, 2024: member

Note that #31196 proposes to drop processNewBlock() from the Mining IPC interface, which straight-forwardly uses ProcessNewBlock. The interface does still use it via submitSolution().

in test/functional/mining_basic.py:243 in 6993836cb6 outdated

236@@ -237,11 +237,19 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
237         bad_block = copy.deepcopy(block)
238         bad_block.vtx[0].vin[0].prevout.hash += 1
239         bad_block.vtx[0].rehash()
240+        self.log.info("getblocktemplate: Test block with missing coinbase")
241         assert_template(node, bad_block, 'bad-cb-missing')
242 
243+        self.log.info("submitblock: Test empty block")

Sjors commented at 3:28 pm on November 21, 2024:

6993836cb6488afa48922f19778ecef6d122bb69 nit: adding these log messages could be its own commit to de-obfuscate what it’s actually testing.

Sjors commented at 3:55 pm on November 21, 2024: member

Something I noticed while looking at df7ad9223025b5d75e27813ce3b89ef6fc431805

The release notes of 0.18 “promise” that the RPC never returns duplicate-invalid:

0- The `submitblock` RPC previously returned the reason a rejected block
1  was invalid the first time it processed that block, but returned a
2  generic "duplicate" rejection message on subsequent occasions it
3  processed the same block.  It now always returns the fundamental
4  reason for rejecting an invalid block and only returns "duplicate" for
5  valid blocks it has already accepted.

Strangely however mining_basic.py has two checks for duplicate-invalid. Those were introduced were introduced in the same v0.18 release: commit fa6ab8ada14dd265e224496440ab0b5d99dfd0ef of #13983.

Update: the real behaviour seems to be that we give the real reason for early checks, but return duplicate-invalid if later checks failed previously. The change in df7ad9223025b5d75e27813ce3b89ef6fc431805 seems compatible with that.

in doc/release-notes-31175.md:5 in 3c6eeaf39e outdated

0@@ -0,0 +1,8 @@
1+RPC
2+---
3+
4+Duplicate blocks submitted with `submitblock` will now persist their block data
5+even pruning. If pruning is activated, the data will be pruned again eventually

Sjors commented at 4:26 pm on November 21, 2024:

3c6eeaf39e01a599829648ecd34990540835962f: even if it was previously pruned.

Sjors approved

Sjors commented at 4:32 pm on November 21, 2024: member

utACK 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5

It would be good to test the new pruned node behavior, in a way similar to rpc_getblockfrompeer.py.

DrahtBot requested review from mzumsande on Nov 21, 2024

instagibbs commented at 5:39 pm on November 21, 2024: member

minimal test case for pruning, feel free to take: https://gist.github.com/instagibbs/cb1f7cfbd0a70e8b5393e457c7a959e6

rpc: Remove submitblock coinbase pre-check

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.

Add some functional test cases to cover the previous conditions that
lead to a "Block does not start with a coinbase" json rpc error being
returned.

---

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.

36dbebafb9

rpc: Remove submitblock invalid-duplicate precheck

ProcessNewBlock fails if an invalid duplicate block is passed in through
its call to AcceptBlock and AcceptBlockHeader. The failure in
AcceptBlockHeader makes AcceptBlock return early. This makes the
pre-check in submitblock redundant.

---

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.

e62a8abd7d

rpc: Remove submitblock duplicate pre-check

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.

A side effect of this change is that a duplicate block is persisted
again on disk even when pruning is activated. This is similar to the
behaviour with getblockfrompeer. Add a release note for this change in
behaviour.

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.

---

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.

1f7fc73825

tests: Add functional test for submitting a previously pruned block

This tests the new submitblock behaviour that is introduced in the
previous commit: Submitting a previously pruned block should persist the
block's data again.

bb53ce9bda

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.

73db95c65c

TheCharlatan force-pushed on Nov 21, 2024

TheCharlatan commented at 9:23 pm on November 21, 2024: contributor

Thank you for the reviews and the test case @instagibbs,

Updated 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5 -> 73db95c65c1d372822166045ca8b9f173d5fd883 (submitblock_prechecks_3 -> submitblock_prechecks_4, compare)

Addressed @Sjors’ comment, improved the logging, but did not split it into its own commit. Should be clear enough with the other changes now.
Addressed @Sjors’ comment, fixed typo.
Added suggested patch from @instagibbs in its own commit.

Sjors commented at 8:31 am on November 22, 2024: member

re-utACK 73db95c65c1d372822166045ca8b9f173d5fd883

I also checked that submitblock in the new test fails with duplicate if you revert 1f7fc738255205a64374686aca9a4c53089360f1.

instagibbs approved

instagibbs commented at 4:41 pm on November 22, 2024: member

ACK 73db95c65c1d372822166045ca8b9f173d5fd883

Not an expert here but motivation makes a lot of sense, and good to remove such indirect code from past covert fixes

in src/rpc/mining.cpp:1022 in 1f7fc73825 outdated

1015@@ -1016,17 +1016,6 @@ static RPCHelpMan submitblock()
1016     }
1017 
1018     ChainstateManager& chainman = EnsureAnyChainman(request.context);
1019-    uint256 hash = block.GetHash();
1020-    {
1021-        LOCK(cs_main);
1022-        const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);

mzumsande commented at 4:29 pm on November 27, 2024:

nit: If, after this PR, we re-submit genesis via submitblock, a different code path will be taken than for non-genesis duplicate blocks: We won’t return early in AcceptBlockHeader, will actually call AddToBlockIndex(), and write an incorrect log message Saw new header hash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 height=0 for each submitblock call. Not a real problem though because nothing fails and why would you do that in the first place.

TheCharlatan commented at 7:31 pm on November 28, 2024:

This is also a debug only log, and it otherwise does not seem to be hitting any hot code paths, so I am not too concerned either.

mzumsande commented at 8:44 pm on November 28, 2024:

it’s unconditional once out of IBD. But allowing untrusted parties access to this rpc is unsafe anyway (can spam the node with low-work blocks since force_processing and min_pow_checked are true for the ProcessNewBlock call).

mzumsande commented at 4:36 pm on November 27, 2024: contributor

ACK 73db95c65c1d372822166045ca8b9f173d5fd883 Reviewed the code (didn’t look closely at bitcoin-chainstate.cpp) and tested it a little bit.

achow101 commented at 10:21 pm on December 3, 2024: member

ACK 73db95c65c1d372822166045ca8b9f173d5fd883

achow101 merged this on Dec 3, 2024

achow101 closed this on Dec 3, 2024

rpc: Remove submitblock pre-checks #31175

Code Coverage & Benchmarks

Reviews

Conflicts