Followups from #30200
Fixes:
std::unique_ptr needs #include <memory> (noticed while working on #30332, which has fewer includes than its parent PR that I originally tested with)Refactor:
state to the end of testBlockValidity, see #30200 (review)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
44@@ -44,6 +45,7 @@ class Mining
45 * @returns a block template
46 */
47 virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
48+
tested ACK 22fe97a5cc612f438ec6280757487934501bf527
Manually validated all instances of testBlockValidity in the entire code base are having their parameter list updated with state as their last output parameter using good old git grep -n testBlockValidity. Now, the interface can be used with the libmultiprocess code generator!
As for the standard testing, I got the pr from upstream, built and run all unit and functional tests including the entire extended test harness and all tests pass.
(Special thanks for incorporating that empty line, it makes eyeballing of different methods pretty easier to spot even for people with poorer eyesight!)
ACK 22fe97a5cc612f438ec6280757487934501bf527
Changes look straightforward. Took a quick look at the other interface functions in mining.h, and the function arguments look to all now be in the in/inout/out order. Also manually ran getblocktemplate RPC on signet and on regtest (after broadcasting a few transactions). All looked well.
389@@ -390,7 +390,7 @@ static RPCHelpMan generateblock()
390 LOCK(cs_main);
It is also acquired within miner.testBlockValidity (and in miner.createNewBlock) so maybe is now unnecessary outside the Miner interface?
createNewBlock. Will look at the other occurances too.
The lock at line 390 has been there from the introduction of the generateblock RPC call in #17693. I don’t see any discussion of it in the reviews.
I think it’s because assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip()); inside of TestBlockValidity. Added 18d6b0f86769f89738a3b08aa8152a8479cef5d4 to explain that, and also make testBlockValidity require the lock.
@maflcko I fixed the commit reference. Still looking at the other lock you mentioned.
Actually that assert isn’t the problem, it merely checks that the chainman().ActiveChainstate() and chainman().ActiveChain().Tip() that we passed in match.
The problem is deeper down the call stack, where TestBlockValidity calls ConnectBlock. That takes a viewNew argument which is chainstate.CoinsTip(), and our proposed block (pindex), and then it does assert(hashPrevBlock == view.GetBestBlock()); where hashPrevBlock = pindex->pprev. In other words, if our proposed block no longer points to the tip it crashes.
Pushed c8343e29a66082b37c37ee0ff8f7433b97eea9b1 to deal with this (replaces https://github.com/bitcoin/bitcoin/commit/18d6b0f86769f89738a3b08aa8152a8479cef5d4). This drops both locks from the RPC code.
An alternative that would actually prevent the tip from updating, rather than fail the RPC call, is to hold the lock even longer. But callers need to handle failure anyway since testBlockValidity can fail for other reasons.
Also note that with the original two separate LOCK(cs_main) calls the tip could have been updated before testBlockValidity anyway, so this is probably a bug fix.
probably a bug fix.
In test-only code. But seems fine to fix, if the fix is correct.
re: #30335 (review)
What is this lock used for?
Marking this thread resolved as the lock is now removed in later commit c8343e29a66082b37c37ee0ff8f7433b97eea9b1
CHECK_NONFATAL can be used to write shorter code and not pollute the scope with single-use symbols. That is:
0 std::optional<uint256> maybe_tip{miner.getTipHash()};
1 CHECK_NONFATAL(maybe_tip);
2 uint256 tip{maybe_tip.value()};
can be:
0 uint256 tip{CHECK_NONFATAL(miner.getTipHash()).value()};
Needed for std::unique_ptr
869@@ -870,9 +870,10 @@ class MinerImpl : public Mining
870 return context()->mempool->GetTransactionsUpdated();
871 }
872
873- bool testBlockValidity(BlockValidationState& state, const CBlock& block, bool check_merkle_root) override
874+ bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) override
.h file.
I initially added it only to the header, but that resulted in:
0node/interfaces.cpp:875:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
1 AssertLockHeld(cs_main);
2 ^
Should I add it in both? Or declare the override first (which would need a new header).
876- LOCK(::cs_main);
877- return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root);
878+ LOCK(cs_main);
879+ CBlockIndex* tip{chainman().ActiveChain().Tip()};
880+ // Fail if the tip updated before the lock was taken
881+ if (block.hashPrevBlock != tip->GetBlockHash()) return false;
state will be valid, no? So this may be confusing?
In commit “Have testBlockValidity hold cs_main instead of caller” (c8343e29a66082b37c37ee0ff8f7433b97eea9b1)
re: #30335 (review)
statewill be valid, no? So this may be confusing?
It seems like it would be better to do something like:
0if (block.hashPrevBlock != tip->GetBlockHash()) {
1 state.Error("Block does not connect to current chain tip.")
2 return false;
3}
871@@ -872,8 +872,12 @@ class MinerImpl : public Mining
872
873 bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) override
874 {
875- LOCK(::cs_main);
876- return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root);
877+ LOCK(cs_main);
878+ CBlockIndex* tip{chainman().ActiveChain().Tip()};
879+ // Fail if the tip updated before the lock was taken
880+ if (block.hashPrevBlock != tip->GetBlockHash()) return false;
nit: In the RPC error you’ll give state.ToString() as the failure reason, but if this tip check fails that’ll be the default BLOCK_RESULT_UNSET.
better suggestion #30335 (review) Seems correct to fail if the tip has updated to avoid building a fork on an older block, but I can’t see a BlockValidationResult that would make sense here other than maybe stretching the meaning of BLOCK_INVALID_HEADER – perhaps you could add another that only makes sense in a templating context like BLOCK_NOT_TIP (or wrap BlockValidationState in something like BlockTemplateValidationState allowing for other template-specific reults but that might be too messy?)
880@@ -881,7 +881,6 @@ class MinerImpl : public Mining
881 BlockAssembler::Options options;
882 ApplyArgsManOptions(gArgs, options);
883
884- LOCK(::cs_main);
In commit “Drop unneeded lock from createNewBlock” (1e4918b8f3af20577acdc9f201af44153e29bcb3)
Commit message could mention the reason the lock is not required in CreateNewBlock, which is just that CreateNewBlock already locks cs_main internally.
Code review ACK c8343e29a66082b37c37ee0ff8f7433b97eea9b1
Nice changes getting rid of some over-broad and recursive uses of cs_main.
This was added in 4bf2e361da1964f7c278b4939967a0e5afde20b0, but
BlockAssembler::CreateNewBlock already locks cs_main internally.
The goal of interfaces is to eventually run in their own process,
so we can't use EXCLUSIVE_LOCKS_REQUIRED in their declaration.
However TestBlockValidaty will crash (in its call to ConnectBlock)
if the tip changes from under the proposed block.
Have the testBlockValidity implementation hold the lock instead,
and non-fatally check for this condition.
Tested ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed
Code reviewed and validated all the initial refactor and appended fix changes as they were discovered during the course of this PR.
Cleaned up and rebuilt this commit running the unit, functional and extended tests that all pass.
662@@ -667,9 +663,7 @@ static RPCHelpMan getblocktemplate()
663 ChainstateManager& chainman = EnsureChainman(node);
664 Mining& miner = EnsureMining(node);
665 LOCK(cs_main);
666- std::optional<uint256> maybe_tip{miner.getTipHash()};
667- CHECK_NONFATAL(maybe_tip);
668- uint256 tip{maybe_tip.value()};
669+ uint256 tip{CHECK_NONFATAL(miner.getTipHash()).value()};
tip is assigned again.
