Mining interface followups, reduce cs_main locking, test rpc bug fix #30335

Sjors · 2024-06-25T11:38:04Z

Followups from #30200

Fixes:

std::unique_ptr needs #include <memory> (noticed while working on Stratum v2 connman #30332, which has fewer includes than its parent PR that I originally tested with)
Drop lock from createNewBlock that was spuriously added
Have testBlockValidity hold cs_main instead of caller (also fixes a race condition in test-only code)

Refactor:

Use CHECK_NONFATAL to avoid single-use symbol (refactor)
move output argument state to the end of testBlockValidity, see Introduce Mining interface #30200 (comment)

DrahtBot · 2024-06-25T11:38:07Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	AngusP, itornaza, ryanofsky
Stale ACK	tdb3

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:

#29432 (Stratum v2 Template Provider (take 3) by Sjors)

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.

src/interfaces/mining.h

itornaza

tested ACK 22fe97a

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!)

AngusP

Tested ACK 22fe97a

tdb3

ACK 22fe97a

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.

maflcko

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()};

src/rpc/mining.cpp

Needed for std::unique_ptr

src/node/interfaces.cpp

maflcko · 2024-06-26T12:08:48Z

src/node/interfaces.cpp

+        LOCK(cs_main);
+        CBlockIndex* tip{chainman().ActiveChain().Tip()};
+        // Fail if the tip updated before the lock was taken
+        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" (c8343e2)

re: #30335 (comment)

state will 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; }

src/node/interfaces.cpp

ryanofsky

Code review ACK c8343e2

Nice changes getting rid of some over-broad and recursive uses of cs_main.

src/node/interfaces.cpp

ryanofsky · 2024-06-26T13:39:30Z

src/node/interfaces.cpp

+        LOCK(cs_main);
+        CBlockIndex* tip{chainman().ActiveChain().Tip()};
+        // Fail if the tip updated before the lock was taken
+        if (block.hashPrevBlock != tip->GetBlockHash()) return false;


In commit "Have testBlockValidity hold cs_main instead of caller" (c8343e2)

re: #30335 (comment)

state will 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; }

ryanofsky · 2024-06-26T13:47:34Z

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 4bf2e36, 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.

Sjors · 2024-06-27T07:04:47Z

Addressed feedback and expanded PR description.

AngusP

Code Review ACK a74b0f9

itornaza

Tested ACK a74b0f9

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.

ryanofsky

Code review ACK a74b0f9. Just new error string is added since last review, and a commit message was updated

maflcko · 2024-06-28T07:20:02Z

src/rpc/mining.cpp

-    std::optional<uint256> maybe_tip{miner.getTipHash()};
-    CHECK_NONFATAL(maybe_tip);
-    uint256 tip{maybe_tip.value()};
+    uint256 tip{CHECK_NONFATAL(miner.getTipHash()).value()};


nit: The same can be done below where tip is assigned again.

Added to #30356.

…ptions c504b69 refactor: add coinbase constraints to BlockCreateOptions (Sjors Provoost) 6b4c817 refactor: pass BlockCreateOptions to createNewBlock (Sjors Provoost) 323cfed refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost) Pull request description: When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs. At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually [produced an invalid block](https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426) which exceeded the sigops limit. The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend to add to the coinbase outputs. Specifically the `CoinbaseOutputDataSize` message which is part of the [Template Distribution Protocol](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server) has a field `coinbase_output_max_additional_size`. A proposed change to the spec adds the max additional sigops as well: stratum-mining/sv2-spec#86. Whether that change makes it into the spec is not important though, as adding both to `BlockAssembler::Options` makes sense. The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required. The second commit introduces BlockCreateOptions, with just `use_mempool`. The thirds commit adds `coinbase_max_additional_weight` and `coinbase_output_max_additional_sigops` to `BlockCreateOptions`. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432. ACKs for top commit: itornaza: tested ACK c504b69 ryanofsky: Code review ACK c504b69 ismaelsadeeq: Code review ACK c504b69 Tree-SHA512: de2fa085f47048c91d95524e03f909f6f27f175c1fefa3d6106445e7eb5cf5b710eda6ea5b641cf3b4704a4e4e0181a0c829003b9fd35465f2a46167e5d64487

Sjors mentioned this pull request Jun 25, 2024

Introduce Mining interface #30200

Merged

Sjors force-pushed the 2024/06/mining-interface-followups branch from 2eab0eb to 22fe97a Compare June 25, 2024 11:42

Sjors commented Jun 25, 2024

View reviewed changes

src/interfaces/mining.h Show resolved Hide resolved

itornaza approved these changes Jun 25, 2024

View reviewed changes

AngusP approved these changes Jun 25, 2024

View reviewed changes

tdb3 approved these changes Jun 25, 2024

View reviewed changes

DrahtBot mentioned this pull request Jun 26, 2024

Stratum v2 Template Provider (take 3) #29432

Closed

24 tasks

maflcko reviewed Jun 26, 2024

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

Sjors added 2 commits June 26, 2024 12:24

Add missing include for mining interface

83a9bef

Needed for std::unique_ptr

refactor: testBlockValidity make out argument last

75ce763

Sjors force-pushed the 2024/06/mining-interface-followups branch 2 times, most recently from d23968e to 18d6b0f Compare June 26, 2024 10:58

maflcko reviewed Jun 26, 2024

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/06/mining-interface-followups branch from 18d6b0f to c8343e2 Compare June 26, 2024 11:44

maflcko reviewed Jun 26, 2024

View reviewed changes

AngusP reviewed Jun 26, 2024

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Jun 26, 2024

View reviewed changes

DrahtBot requested review from AngusP, tdb3 and itornaza June 26, 2024 13:44

Sjors changed the title ~~Mining interface followups~~ Mining interface followups, reduce cs_main locking, test rpc bug fix Jun 27, 2024

Sjors added 3 commits June 27, 2024 08:56

Drop unneeded lock from createNewBlock

5fb2b70

This was added in 4bf2e36, but BlockAssembler::CreateNewBlock already locks cs_main internally.

refactor: use CHECK_NONFATAL to avoid single-use symbol

f6dc6db

Sjors force-pushed the 2024/06/mining-interface-followups branch from c8343e2 to a74b0f9 Compare June 27, 2024 07:00

AngusP approved these changes Jun 27, 2024

View reviewed changes

DrahtBot requested a review from ryanofsky June 27, 2024 13:50

itornaza approved these changes Jun 27, 2024

View reviewed changes

ryanofsky approved these changes Jun 27, 2024

View reviewed changes

ryanofsky merged commit 2f6dca4 into bitcoin:master Jun 27, 2024

Sjors deleted the 2024/06/mining-interface-followups branch June 28, 2024 06:38

maflcko reviewed Jun 28, 2024

View reviewed changes

Sjors mentioned this pull request Jun 28, 2024

refactor: add coinbase constraints to BlockAssembler::Options #30356

Merged

Theschorpioen approved these changes Aug 6, 2024

View reviewed changes

Mining interface followups, reduce cs_main locking, test rpc bug fix #30335

Mining interface followups, reduce cs_main locking, test rpc bug fix #30335

Uh oh!

Conversation

Sjors commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

itornaza left a comment

Choose a reason for hiding this comment

Uh oh!

AngusP left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jun 26, 2024

Uh oh!

Sjors commented Jun 27, 2024

Uh oh!

AngusP left a comment

Choose a reason for hiding this comment

Uh oh!

itornaza left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Jun 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 25, 2024 •

edited

Loading

DrahtBot commented Jun 25, 2024 •

edited

Loading