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)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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 | +
Added blank line here for consistency.
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!)
Tested ACK 22fe97a5cc612f438ec6280757487934501bf527
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);
What is this lock used for?
Same in line 374
It is also acquired within miner.testBlockValidity (and in miner.createNewBlock) so maybe is now unnecessary outside the Miner interface?
I pushed 1f0559e9954253c23af7ee9b49e57eb3d7a45bea to drop it from 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:
std::optional<uint256> maybe_tip{miner.getTipHash()};
CHECK_NONFATAL(maybe_tip);
uint256 tip{maybe_tip.value()};
can be:
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
Lock annotations in the cpp file will be ignored by the compiler, because it usually only sees the .h file.
I initially added it only to the header, but that resulted in:
node/interfaces.cpp:875:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
AssertLockHeld(cs_main);
^
Should I add it in both? Or declare the override first (which would need a new header).
Well, I don't even understand the big picture. Isn't the point of interfaces to have (possibly) multiple processes? How would one even take and release a lock in another process?
Good point, it doesn't make sense to require the lock. Will try something else.
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?
just a nit, because it only affects test-only code, but wanted to leave the comment.
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:
if (block.hashPrevBlock != tip->GetBlockHash()) {
state.Error("Block does not connect to current chain tip.")
return false;
}
Taken
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?)
Currently if this happens as you said #30335 (review) it looks like we'd have crashed which is fun
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.
Reworded
Code review ACK c8343e29a66082b37c37ee0ff8f7433b97eea9b1
Nice changes getting rid of some over-broad and recursive uses of cs_main.
It would be good to mention that the are some changes to cs_main locking (including a potential bugfix) in the PR description, otherwise this should be a refactoring with no changes in behavior.
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.
Addressed feedback and expanded PR description.
Code Review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed
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.
Code review ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed. Just new error string is added since last review, and a commit message was updated
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()};
nit: The same can be done below where tip is assigned again.
