Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Jun 28, 2024

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 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 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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 28, 2024

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 itornaza, ryanofsky, ismaelsadeeq
Concept NACK luke-jr
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:

  • #30475 (Stratum v2 Template Provider common functionality by Sjors)
  • #30443 (Introduce waitFeesChanged() mining interface by Sjors)
  • #30440 (Have createNewBlock() return a BlockTemplate interface by Sjors)
  • #30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
  • #30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals by Sjors)
  • #30391 (BlockAssembler: return selected packages vsize and feerate by ismaelsadeeq)
  • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
  • #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.

Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK
This adds flexibility for SV2, and having less magic numbers is better. I support adding a configuration parameter for this (would prevent others from needing to fork).

Left a couple small nits for now, but plan to test/exercise in more detail.

@@ -42,9 +43,13 @@ class Mining
*
* @param[in] script_pub_key the coinbase output
* @param[in] use_mempool set false to omit mempool transactions
* @param[in] coinbase_output_max_additional_size maximum additional serialized bytes which the pool will add in coinbase transaction outputs
Copy link
Contributor

@tdb3 tdb3 Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is it comprehensive using the term "pool" here, or would it be better to keep things generic (and perhaps mention a pool as an example)? This is reserving space in the template for the coinbase and the entity requesting the template could be a pool or a solo miner (albeit less commonly), right? Maybe something like maximum additional serialized bytes the block requestor can add in coinbase transaction outputs (for example, to enable pool payouts).

nit: weight and bytes seem to be used interchangeably here and in the function definition. It might prevent confusion to stick to one nomenclature.

Copy link
Member Author

@Sjors Sjors Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree something like "block requestor" is more generic than "pool", but also more confusing.

weight and bytes seem to be used interchangeably here and in the function definition

These bytes all go in the output which doesn't have a witness discount, so each byte counts as 4 weight units. I think we should prefer the term "bytes", but make sure it's used correctly since we use weight units for internal accounting.

Copy link
Member Author

@Sjors Sjors Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also: https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputdatasize-client---server

Which has a few more hairy details:

The Template Provider MUST NOT provide NewMiningJob messages which would represent consensus-invalid blocks once this additional size — along with a maximally-sized (100 byte) coinbase field — is added. Further, the Template Provider MUST consider the maximum additional bytes required in the output count variable-length integer in the coinbase transaction when complying with the size limits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up renaming coinbase_output_max_additional_size to coinbase_max_additional_weight to account for these issues, see below.

@TheCharlatan
Copy link
Contributor

TheCharlatan commented Jun 30, 2024

I picked libbitcoin_util instead of libbitcoin_common to avoid a future dependency of the Stratum v2 Template Provider (#29432) on libbitcoin_common. The latter goal requires additional changes and is mostly just nice to have (the important bit is not depending on libbitcoin_node).

So I guess it should ideally only have the kernel as a dependency? It is not quite clear to me yet how this is going to be achieved. The current mining interface requires knowledge of a CBlockTemplate, which I don't think should be part of the kernel in the future.

EDIT: To be clear, since the constants are given a new header, there is no new hard library dependency introduced no matter which directory you choose to put them into. It just feels weird to put something very specific to the inner workings of our mining code into a directory called util.

@Sjors
Copy link
Member Author

Sjors commented Jul 1, 2024

@TheCharlatan happy to put the constants somewhere else... see Sjors#47 for an attempt at introducing libbitcoin_net, which might reduce the dependencies of a future libbitcoin_sv2 to libbitcoin_net, libbitcoin_crypto and libbitcoin_util. I got stuck there on where to put XOnlyPubKey and a few other things.

A similar problem might exist for CBlockTemplate if I actually tried to make libbitcoin_sv2, which I haven't done yet.

@Sjors Sjors force-pushed the 2024/06/coinbase-constraints branch from 4c57f27 to 3d45921 Compare July 1, 2024 10:30
@Sjors
Copy link
Member Author

Sjors commented Jul 1, 2024

I renamed coinbase_output_max_additional_size to coinbase_max_additional_weight. With the current Stratum v2 spec the latter could be calculated as follows:

coinbase_max_additional_weight = (coinbase_output_max_additional_size + 100 + 0 + 2) * 4

  • coinbase_output_max_additional_size * 4: Coinbase outputs are not part of the witness so their weight is simply the number of bytes specificied in the CoinbaseOutputDataSize message times 4.

  • 100 * 4: the spec also requires taking into account a "maximally-sized (100 byte) coinbase field", which refers to the scriptSig of the coinbase. It currently doesn't allow for setting a custom coinbase witness.

  • 0 * 4: a CompactSize encodes the size of the above "coinbase field" (the spec is ambiguous whether this is included). Given the maximum of 100 its always encoded with 1 byte, so no additional weight units are needed.

  • 2 * 4: "Further, the Template Provider MUST consider the maximum additional bytes required in the output count variable-length integer in the coinbase transaction when complying with the size limits." - this is tricky because we don't know how many outputs the pool intends to add, and we might want to add an arbitrary number of outputs ourselves. Since there can't be more than 1 million outputs, 3 bytes is always enough to encode the number of outputs. So that's 2 additional bytes in the worst case.

The spec should probably be clarified a bit, cc @TheBlueMatt, @Fi3.

In any case, tracking coinbase reserved space in term of weight units here should be enough to support Stratum v2 later, and even allow for using the coinbase witness.

@Sjors
Copy link
Member Author

Sjors commented Jul 4, 2024

Rebased after #30324.

@TheCharlatan
Copy link
Contributor

Is there a reason why

BlockAssembler::Options options;
ApplyArgsManOptions(gArgs, options);

has to be inside the block assembler class? Why not construct them externally and pass a reference of the options to the BlockAssembler? This seems a bit more efficient, since the options don't have to be constructed and the arguments not read every time a template is retrieved.

@Sjors
Copy link
Member Author

Sjors commented Jul 4, 2024

cc @glozow who added this as part of #26695.

@glozow
Copy link
Member

glozow commented Jul 4, 2024

Is there a reason why

BlockAssembler::Options options;
ApplyArgsManOptions(gArgs, options);

has to be inside the block assembler class?

It doesn't need to be inside BlockAssembler. Prior to #26695, the ctor was using the global gArgs to decide on parameters (yuck!). We didn't have the node/*args kernel/*opts conventions at the time and I was pretty happy adding an ApplyArgsMan that didn't do that.

Why not construct them externally and pass a reference of the options to the BlockAssembler?

That sounds fine.

@luke-jr
Copy link
Member

luke-jr commented Jul 6, 2024

Weak approach NACK. This is just limiting the max block weight, which is already a configurable parameter. Having two params for the same thing seems like a poor way to do it. I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable.

Instead, maybe just perform the block weight cap on initialization rather than block assembly?

@Sjors
Copy link
Member Author

Sjors commented Jul 8, 2024

This is just limiting the max block weight

This had me confused initially, and I tried to combine them, but there's really two different things:

  1. The max block weight demanded by the user via -blockmaxweight
  2. Weight units reserved for the coinbase

With stratum v1 the distinction isn't very important. We just subtract (2) from (1). We could indeed do that at initialization.

But with Stratum v2 (2) is a per-client setting communicated to us (the Template Provider) via the CoinbaseOutputDataSize message. We can set it at connection time, but not during node initialization.

I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable.

The fact that Stratum v2 makes this configurable introduces that risk. But removing that ability from the protocol would probably just cause miners to roll their own extension and do it anyway. At 500 sats/byte and $50K per coin, squeezing in an extra 1000 vbyte gets you $250 per block. Pennies in front of steamrollers perhaps, but we've seen miners do this with the current code.

Why not construct them externally and pass a reference of the options to the BlockAssembler?

That sounds fine.

Happy to take patches.

Copy link
Contributor

@itornaza itornaza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK d89637e

Giving the option to assign the values of coinbase_max_additional_weight and coinbase_output_max_additional_sigops needed by the Stratum V2 protocol is essential for its integration. I understand the risks involved by having two parameters for the same thing:

1. The max block weight demanded by the user via -blockmaxweight
2. Weight units reserved for the coinbase

but it seems unavoidable at a first glance due to their different invocation domains. During the node initialisation for the former and connection time for the later.

Left some small nit that only applies if the intended name for the variable was coinbase_max_additional_weight instead of coinbase_max_addition_weight

@@ -42,9 +43,13 @@ class Mining
*
* @param[in] script_pub_key the coinbase output
* @param[in] use_mempool set false to omit mempool transactions
* @param[in] coinbase_max_addition_weight maximum additional weight which the pool will add to the coinbase transaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it you wanted to rename it to coinbase_max_additional_weight instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@ryanofsky
Copy link
Contributor

Code review ACK e67e466, but I think the current implementation is too complicated and has too many layers of indirection.

Would suggest applying the following patch and squashing the PR down to a single commit which would be just +41/-16 with the suggested changes:

diff

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
   util/hasher.h \
   util/insert.h \
   util/macros.h \
-  util/mining.h \
   util/moneystr.h \
   util/overflow.h \
   util/overloaded.h \
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -5,10 +5,11 @@
 #ifndef BITCOIN_INTERFACES_MINING_H
 #define BITCOIN_INTERFACES_MINING_H
 
+#include <node/types.h>
+#include <uint256.h>
+
 #include <memory>
 #include <optional>
-#include <uint256.h>
-#include <util/mining.h>
 
 namespace node {
 struct CBlockTemplate;
@@ -42,14 +43,10 @@ public:
      * Construct a new block template
      *
      * @param[in] script_pub_key the coinbase output
-     * @param[in] use_mempool set false to omit mempool transactions
-     * @param[in] coinbase_max_additional_weight maximum additional weight which the pool will add to the coinbase transaction
-     * @param[in] coinbase_output_max_additional_sigops maximum additional sigops which the pool will add in coinbase transaction outputs
+     * @param[in] options options for creating the block
      * @returns a block template
      */
-    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true,
-                                                                 size_t coinbase_max_additional_weight = DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT,
-                                                                 size_t coinbase_output_max_additional_sigops = DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS) = 0;
+    virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options={}) = 0;
 
     /**
      * Processes new block. A valid new block is automatically relayed to peers.
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -883,25 +883,11 @@ public:
         return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root);
     }
 
-    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool,
-                                                   size_t coinbase_max_additional_weight,
-                                                   size_t coinbase_output_max_additional_sigops) override
+    std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, const BlockCreateOptions& options) override
     {
-        BlockAssembler::Options options;
-        ApplyArgsManOptions(gArgs, options);
-
-        Assume(coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
-        options.coinbase_max_additional_weight = coinbase_max_additional_weight;
-
-        Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
-        options.coinbase_output_max_additional_sigops = coinbase_output_max_additional_sigops;
-
-        // The BlockAssembler constructor calls ClampOptions which clamps
-        // nBlockMaxWeight between coinbase_output_max_additional_size and
-        // DEFAULT_BLOCK_MAX_WEIGHT.
-        // In other words, coinbase (reserved) outputs can safely exceed
-        // -blockmaxweight, but the rest of the block template will be empty.
-        return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key);
+        BlockAssembler::Options assemble_options{options};
+        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
+        return BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(script_pub_key);
     }
 
     NodeContext* context() override { return &m_node; }
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -59,14 +59,17 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 
 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 {
+    Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
+    Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
     // Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
+    // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
     options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
     return options;
 }
 
 BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options)
     : chainparams{chainstate.m_chainman.GetParams()},
-      m_mempool{mempool},
+      m_mempool{options.use_mempool ? mempool : nullptr},
       m_chainstate{chainstate},
       m_options{ClampOptions(options)}
 {
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -6,6 +6,7 @@
 #ifndef BITCOIN_NODE_MINER_H
 #define BITCOIN_NODE_MINER_H
 
+#include <node/types.h>
 #include <policy/policy.h>
 #include <primitives/block.h>
 #include <txmempool.h>
@@ -20,8 +21,6 @@
 #include <boost/multi_index/tag.hpp>
 #include <boost/multi_index_container.hpp>
 
-#include <util/mining.h>
-
 class ArgsManager;
 class CBlockIndex;
 class CChainParams;
@@ -155,24 +154,13 @@ private:
     Chainstate& m_chainstate;
 
 public:
-    struct Options {
+    struct Options : BlockCreateOptions {
         // Configuration parameters for the block size
         size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT};
         CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
         // Whether to call TestBlockValidity() at the end of CreateNewBlock().
         bool test_block_validity{true};
         bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE};
-        /**
-         * The maximum additional weight which the pool will add to the coinbase
-         * scriptSig, witness and outputs. This must include any additional
-         * weight needed for larger CompactSize encoded lengths.
-         */
-        size_t coinbase_max_additional_weight{DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT};
-        /**
-         * The maximum additional sigops which the pool will add in coinbase
-         * transaction outputs.
-         */
-        size_t coinbase_output_max_additional_sigops{DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS};
     };
 
     explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options);
--- a/src/node/types.h
+++ b/src/node/types.h
@@ -13,6 +13,8 @@
 #ifndef BITCOIN_NODE_TYPES_H
 #define BITCOIN_NODE_TYPES_H
 
+#include <cstddef>
+
 namespace node {
 enum class TransactionError {
     OK, //!< No error
@@ -24,6 +26,24 @@ enum class TransactionError {
     MAX_BURN_EXCEEDED,
     INVALID_PACKAGE,
 };
+
+struct BlockCreateOptions {
+    /**
+     * Set false to omit mempool transactions
+     */
+    bool use_mempool{true};
+    /**
+     * The maximum additional weight which the pool will add to the coinbase
+     * scriptSig, witness and outputs. This must include any additional
+     * weight needed for larger CompactSize encoded lengths.
+     */
+    size_t coinbase_max_additional_weight{4000};
+    /**
+     * The maximum additional sigops which the pool will add in coinbase
+     * transaction outputs.
+     */
+    size_t coinbase_output_max_additional_sigops{400};
+};
 } // namespace node
 
 #endif // BITCOIN_NODE_TYPES_H
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -371,7 +371,7 @@ static RPCHelpMan generateblock()
 
     ChainstateManager& chainman = EnsureChainman(node);
     {
-        std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)};
+        std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, {.use_mempool = false})};
         if (!blocktemplate) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         }
deleted file mode 100644
--- a/src/util/mining.h
+++ /dev/null
@@ -1,13 +0,0 @@
-// Copyright (c) 2024 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#ifndef BITCOIN_UTIL_MINING_H
-#define BITCOIN_UTIL_MINING_H
-
-/** Default for coinbase_max_additional_weight */
-static constexpr unsigned int DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT{4000};
-/** Default for coinbase_output_max_additional_sigops */
-static constexpr unsigned int DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS{400};
-
-#endif // BITCOIN_UTIL_MINING_H

Suggested changes:

  • Make createNewBlock use an options struct so it easier and safer to use (less likely to have bugs from mixing up the order of integer parameters).
  • Consolidates documentation for the options in one header instead of three headers.
  • Avoids adding util/mining.h header since the options more logically belong to the node library than the util library.
  • Moves implementation of mining options from node/interfaces.cpp to node/miner.cpp. The interfaces.cpp file is just meant to contain glue code exposing private implementations as public interfaces. Ideally, the interface methods should be 1-3 lines long and not implement real functionality, which belongs in other places like validation.cpp and miner.cpp so it can be found and reused more easily and better tested.

@DrahtBot DrahtBot requested a review from itornaza July 15, 2024 13:47
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK c504b69

Comment on lines 62 to 63
Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
Copy link
Contributor

@ryanofsky ryanofsky Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #30356 (comment)

Just to clarify for other reviewers since the names of these macros are confusing: In bitcoin core, assert and Assert macros behave very differently from the assert macro in standard c++. In standard c++ assert statements are not executed at all in release builds, while in bitcoin core they are always executed and always cause the execution to abort if they fail. Other c++ libraries use the term check instead of assert for this behavior.

The Assume macro is more unusual, because it doesn't abort execution in release builds, but does also doesn't get skipped in release builds, so will always evaluate arguments passed to it. It can make sense to use Assume instead of assert when an unexpected condition indicates some kind of bug, but the bug is easy to recover from, so it is worth crashing debug builds but not release builds. Normally it make sense to check the Assume return value and do something like log a warning or fix invalid values, not just ignore the unexpected condition completely.

Copy link
Member

@ismaelsadeeq ismaelsadeeq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK c504b69

The PR description needs to be updated to match the PR.

Left a single question

* Set false to omit mempool transactions
*/
bool use_mempool{true};
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is their a reason we are not adding coinbase_script into the BlockCreateOptions struct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong reason, but the coinbase script is different for every block, unlike the other stuff we pass to BlockCreateOptions. It seems worthy of a positional argument.

Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Jul 18, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Jul 18, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
@Sjors
Copy link
Member Author

Sjors commented Jul 18, 2024

@ismaelsadeeq thanks, updated the description.

@ryanofsky ryanofsky merged commit ef19a19 into bitcoin:master Jul 18, 2024
16 checks passed
@glozow
Copy link
Member

glozow commented Jul 18, 2024

post merge ACK

@Sjors Sjors deleted the 2024/06/coinbase-constraints branch July 18, 2024 16:47
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 18, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit changes the C++ Mining interface without changing the corresponding
Capn'Proto interface. The capnp interface is updated the next commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 18, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 18, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit changes the C++ Mining interface without changing the corresponding
Capn'Proto interface. The capnp interface is updated the next commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 18, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Jul 19, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 24, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 24, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit changes the C++ Mining interface without changing the corresponding
Capn'Proto interface. The capnp interface is updated the next commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 24, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Aug 13, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Aug 13, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 6, 2024
Add interface changes from

bitcoin#30409
bitcoin#30356
bitcoin#30440
bitcoin#30443

This commit updates the Cap'n Proto Mining interface after updating the C++
Mining interface last commit.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 26, 2024
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Apr 26, 2025
…nough to give up" size delta

PR bitcoin#30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:

```diff
             if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
-                    m_options.nBlockMaxWeight - 4000) {
+                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
                 // Give up if we're close to full and haven't succeeded in a while
                 break;
             }
```

But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts.

It doesn't seem very logical to reuse the reserve size for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Apr 26, 2025
…nough to give up" weight delta

PR bitcoin#30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:

```diff
             if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
-                    m_options.nBlockMaxWeight - 4000) {
+                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
                 // Give up if we're close to full and haven't succeeded in a while
                 break;
             }
```

But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts.

It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
achow101 added a commit that referenced this pull request Apr 29, 2025
…lock is full enough to give up" weight delta

524f981 Bugfix: Miner: Don't reuse block_reserved_weight for "block is full enough to give up" weight delta (Luke Dashjr)

Pull request description:

  PR #30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:

  ```diff
               if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
  -                    m_options.nBlockMaxWeight - 4000) {
  +                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
                   // Give up if we're close to full and haven't succeeded in a while
                   break;
               }
  ```

  But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts.

  It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.

ACKs for top commit:
  achow101:
    ACK 524f981
  darosior:
    utACK 524f981
  ismaelsadeeq:
    ACK 524f981

Tree-SHA512: c066debc34a021380424bd21b40444071b736325e41779a41590c2c8a6822ceeaf910fe067817c1dba108210b24c574977b0350b29520502e7af79d3b405928b
w0xlt pushed a commit to w0xlt/bitcoin that referenced this pull request May 7, 2025
…nough to give up" weight delta

PR bitcoin#30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block:

```diff
             if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
-                    m_options.nBlockMaxWeight - 4000) {
+                    m_options.nBlockMaxWeight - m_options.block_reserved_weight) {
                 // Give up if we're close to full and haven't succeeded in a while
                 break;
             }
```

But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts.

It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 5, 2025
@bitcoin bitcoin locked and limited conversation to collaborators Jul 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants