bitcoin / bitcoin/bitcoin

`BlockAssembler` underfill blocks and mixes reserve weights

Open
#35,596 3 comments 4 reactions 0 assignees View on GitHub
Mining
Dominant language
C++
Stars
90.2k
Forks
39.4k
Avg merge
3d 11h
Merged PRs (30d)
91

Description

`BlockAssembler` fills a block template greedily. It pulls the best chunk from the TxGraph builder via `CTxMemPool::GetBlockBuilderChunk`, checks it with `TestChunkBlockLimits` and `TestChunkTransactions`, then either adds the chunk to the template or skips it. After enough consecutive skips near the weight ceiling, it gives up.

There are three issues with the current `BlockAssembler`.

#### 1. `>=` limit checks make the consensus boundary unreachable

`TestChunkBlockLimits` currently rejects chunks that would make the block exactly reach the configured weight or sigops limit:

```cpp
if (nBlockWeight + chunk_feerate.size >= *m_options.block_max_weight) {
return false;
}
if (nBlockSigOpsCost + chunk_sigops_cost >= MAX_BLOCK_SIGOPS_COST) {
return false;
}
```

Consensus rejects blocks only when these limits are exceeded:

* `GetBlockWeight(block) > MAX_BLOCK_WEIGHT` is invalid, so `== MAX_BLOCK_WEIGHT` is valid.
* `nSigOps * WITNESS_SCALE_FACTOR > MAX_BLOCK_SIGOPS_COST` is invalid, so `== MAX_BLOCK_SIGOPS_COST` is valid.

This means a chunk that lands the block exactly on `block_max_weight` or `MAX_BLOCK_SIGOPS_COST` is skipped even though the resulting block would be consensus-valid. As a result, the full weight and sigops allowances are unreachable.

#### 2. The weight check uses sigop-adjusted weight, while accounting uses actual weight

`chunk_feerate.size` from `GetBlockBuilderChunk` is the sigop-adjusted weight stored in TxGraph:

```cpp
FeePerWeight feerate(fee, GetSigOpsAdjustedWeight(GetTransactionWeight(*tx),
sigops_cost, ::nBytesPerSigOp));

int64_t GetSigOpsAdjustedWeight(int64_t weight, int64_t sigop_cost, unsigned int bytes_per_sigop)
{
return std::max(weight, sigop_cost * bytes_per_sigop); // default bytes_per_sigop = 20
}
```

But `AddToBlock` accounts for actual transaction weight:

```cpp
nBlockWeight += entry.GetTxWeight();
```

So `TestChunkBlockLimits` compares sigop-adjusted weight against `block_max_weight`.

This can cause `BlockAssembler` to skip chunks whose actual weight fits, potentially leaving higher-fee transactions out of the template.

This is fixed in https://github.com/bitcoin/bitcoin/pull/35580.

#### 3. Block reserved weight mixes consensus-determined and caller-determined weight

A `CBlock` is a `CBlockHeader` plus `vtx`. Its weight is computed as:

```cpp
weight = GetSerializeSize(TX_NO_WITNESS(block)) * (WITNESS_SCALE_FACTOR - 1)
+ GetSerializeSize(TX_WITH_WITNESS(block));
```

With `WITNESS_SCALE_FACTOR = 4`, this is:

```text
base_bytes * 4 + witness_bytes
```

The block header is 80 base bytes, or 320 WU:

| Field | Type | Bytes |
| ---------------- | ---------: | ----: |
| `nVersion` | `int32_t` | 4 |
| `hashPrevBlock` | `uint256` | 32 |
| `hashMerkleRoot` | `uint256` | 32 |
| `nTime` | `uint32_t` | 4 |
| `nBits` | `uint32_t` | 4 |
| `nNonce` | `uint32_t` | 4 |

`resetBlock` seeds the block with reserved weight. The reserved components include:

| Component | Side | Bytes | Weight |
| ------------------------------------ | ------: | ----------: | --------------: |
| Block header | base | 80 | 320 |
| Tx-count CompactSize | base | 3 | 12 |
| Coinbase marker + flag (`0x00 0x01`) | witness | 2 | 2 |
| Coinbase legacy size | base | `legacy_sz` | `legacy_sz * 4` |
| Coinbase witness reserved value | witness | 34 | 34 |

The problem is that `-blockreservedweight` combines two different categories of weight. Consensus, which is pre-determined all the above except "Coinbase legacy size " which is a user-determined weight. They can instead be categorized into

**consensus reserved weight** (added at the beginning of blockassembler):

* block header: 320 WU
* tx-count CompactSize 12 WU
* coinbase marker + flag: 2 WU
* witness commitment: 34 WU

**user-determined weight** (-blockreservedweight)

coinbase transaction legacy size * 4 (the coinbase transaction serialized with TX_NO_WITNESS)

Issues 1 and 2 are straightforward. Issue 2 already has a PR: https://github.com/bitcoin/bitcoin/pull/35580.

For issue 3, the reserve accounting could be clarified by

i. Reserve the consensus-determined weight inside `BlockAssembler` instead of accounting for it through `-blockreservedweight`.

ii. Redefine `MINIMUM_BLOCK_RESERVED_WEIGHT` so that it represents only the minimum caller-determined coinbase transaction legacy weight.

iii). Redefine `DEFAULT_BLOCK_RESERVED_WEIGHT` as a conservative default for caller-determined coinbase transaction legacy weight.

With issues 1 and 2 fixed, and reserve accounting separated this way, an exactly filled consensus-valid block template could become achievable.

I am curious whether this approach is compatible with clients such as Stratum V2 (I assume it is because most of the fields it needs from the block assembler (CoinbaseTx) size can be known before calling `createNewBlock`) and DATUM, and whether I am missing something.

Contributor guide

Open the contributing guide

Research direction

Start with BlockAssembler::TestChunkBlockLimits, AddToBlock, and resetBlock, then compare their weight accounting with the consensus boundary described here. Check PR 35580 before duplicating work on the adjusted-weight issue. Done means exact weight and sigops limits remain usable while consensus-reserved and caller-determined coinbase weight are accounted for separately.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, blockchain
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.