BIP++1: First pass. #1

xanather · 2022-10-15T03:46:50Z

BIP++1 is only described in the adjusted readme.md.

DO NOT USE YET outside testnets. Read readme.md.

I've only been studying bitcoin core code for a few days. Things I've noticed need looking at:

TODO:

Versionbit 27 tests
Larger CompactBlock support (larger indices). Adjusting bucket checking sizes etc... Potentially need to add feature flagging here to only transmit indices > 3 bytes after activation.
Other buffering, net code constants that may need to be adjusted that aren't immediately apparent.
Not so sure about concurrency locks on new DeploymentActiveAfter/DeploymentActiveAt calls.
All comments I have placed on the PR commits answered.

Please assist me in making this patch more viable before more formal validation takes place. Submit a PR to the BIP++1 branch.

DO NOT USE YET outside testnets. Read readme.md.

xanather · 2022-10-15T04:19:48Z

bitcoin/src/blockencodings.cpp

Line 52 in 5b2bfba

    
           if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_WEIGHT / MIN_SERIALIZABLE_TRANSACTION_WEIGHT)

Non consensus code, immediately increase max size. Larger actual compact block sizes support will come soon.

xanather · 2022-10-15T04:22:35Z

bitcoin/src/coins.cpp

Line 282 in 5b2bfba

    
           static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_WEIGHT / MIN_TRANSACTION_OUTPUT_WEIGHT;

This only affects AccessByTxid, used in DisconnectBlock and gettxproofs. Non-consensus.

xanather · 2022-10-15T04:27:48Z

bitcoin/src/merkleblock.cpp

Line 155 in 5b2bfba

if (nTransactions > MAX_BLOCK_WEIGHT / MIN_TRANSACTION_WEIGHT)

Non consensus code quick validity check. Used by backup and txoutproof only.

xanather · 2022-10-15T04:48:33Z

bitcoin/src/validation.cpp

Line 833 in 1bb1792

if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST)

As mentioned previously, per TX limits aren't changing.

xanather · 2022-10-15T04:53:31Z

bitcoin/src/net.cpp

Line 2689 in 1bb1792

    
           const uint64_t buffer = timeLeftInCycle / std::chrono::minutes{10} * MAX_BLOCK_SERIALIZED_SIZE;

Network IO limit already adjusted before activation. Required when disconnecting blocks near activation. May need to change other constants such as MAX_UPLOAD_TIMEFRAME.

xanather · 2022-10-15T04:56:47Z

bitcoin/src/validation.cpp

Line 4394 in 1bb1792

    
           CBufferedFile blkdat(fileIn, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8, SER_DISK, CLIENT_VERSION);

Read disk buffer will become 1GB in-memory size. Mentioned in the readme. Non-concurrent from first look so would not blow out.

xanather · 2022-10-15T04:16:00Z

src/bitcoin-tx.cpp

    static const unsigned int minTxOutSz = 9;
-    static const unsigned int maxVout = MAX_BLOCK_WEIGHT / (WITNESS_SCALE_FACTOR * minTxOutSz);
+    static const unsigned int maxVout = MAX_BLOCK_WEIGHT_LEGACY / (WITNESS_SCALE_FACTOR * minTxOutSz);


Rationalize: No need to adjust this limit. tx_check.cpp also uses the legacy MAX_BLOCK_WEIGHT for tx sizes. There is enough UTX IN/OUT count in the base protocol size.

xanather · 2022-10-15T04:26:01Z

src/consensus/tx_check.cpp

@@ -16,7 +16,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
    if (tx.vout.empty())
        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vout-empty");
    // Size limits (this doesn't take the witness into account, as that hasn't been checked for malleability)
-    if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)
+    if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT_LEGACY /* Retain old block weight for max sz tx size. */)


Mentioned above, no need to adjust this rule. Feel free to argue otherwise.

xanather · 2022-10-15T04:30:30Z

src/node/miner.cpp

    : chainparams{chainstate.m_chainman.GetParams()},
      m_mempool(mempool),
      m_chainstate(chainstate)
 {
    blockMinFeeRate = options.blockMinFeeRate;
    // Limit weight to between 4K and MAX_BLOCK_WEIGHT-4K for sanity:
-    nBlockMaxWeight = std::max<size_t>(4000, std::min<size_t>(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight));
+    nBlockMaxWeight = std::max<size_t>(4000, std::min<size_t>((fPlusPlusActivated ? MAX_BLOCK_WEIGHT : MAX_BLOCK_WEIGHT_LEGACY) - 4000, options.nBlockMaxWeight));


nBlockMaxWeight will only adjust in block assembler once BIP++1 activated.
Old limit will remain indefinitely until then. After activation and no limit is specified by the miner then 2MB non-weighted will become the default.

xanather · 2022-10-15T04:32:10Z

src/policy/policy.h

@@ -20,7 +20,7 @@ class CFeeRate;
 class CScript;

 /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
-static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
+static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT_LEGACY - 4000};


See comment in miner.cpp. This gets overridden by option value anyway.

xanather · 2022-10-15T04:35:09Z

src/validation.cpp

@@ -3406,7 +3412,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
    // checks that use witness data may be performed here.

    // Size limits
-    if (block.vtx.empty() || block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT || ::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT)
+    const unsigned int maxWeight = fPlusPlusActivated ? MAX_BLOCK_WEIGHT : MAX_BLOCK_WEIGHT_LEGACY;
+    if (block.vtx.empty() || block.vtx.size() * WITNESS_SCALE_FACTOR > maxWeight || ::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > maxWeight)


Contextual flag has been put in this routine. I did not want to move the check into AcceptBlock().

xanather · 2022-10-15T04:35:29Z

src/validation.cpp

@@ -3433,7 +3440,7 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
    {
        nSigOps += GetLegacySigOpCount(*tx);
    }
-    if (nSigOps * WITNESS_SCALE_FACTOR > MAX_BLOCK_SIGOPS_COST)
+    if (nSigOps * WITNESS_SCALE_FACTOR > (fPlusPlusActivated ? MAX_BLOCK_SIGOPS_COST : MAX_BLOCK_SIGOPS_COST_LEGACY))


Same as above.

xanather · 2022-10-15T04:36:25Z

src/node/miner.cpp

@@ -201,7 +202,7 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost
    if (nBlockWeight + WITNESS_SCALE_FACTOR * packageSize >= nBlockMaxWeight) {
        return false;
    }
-    if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) {
+    if (nBlockSigOpsCost + packageSigOpsCost >= (nPlusPlusActivated ? MAX_BLOCK_SIGOPS_COST : MAX_BLOCK_SIGOPS_COST_LEGACY)) {


Block assembly max ops only bumped after activation.

xanather · 2022-10-15T04:50:22Z

src/policy/policy.h

@@ -30,7 +30,7 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{82};
 /** Maximum number of signature check operations in an IsStandard() P2SH script */
 static constexpr unsigned int MAX_P2SH_SIGOPS{15};
 /** The maximum number of sigops we're willing to relay/mine in a single tx */
-static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5};
+static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST_LEGACY / 5};


As mentioned previously, per TX limits aren't changing.

xanather added 2 commits October 14, 2022 20:50

BIP++1: First pass.

a9711a0

DO NOT USE YET outside testnets. Read readme.md.

Updating readme.md.

5b2bfba

Review 1: MAX_STANDARD_TX_SIGOPS_COST should not change.

1bb1792

Adjusting readme.md based on actual logic.

d739461

xanather commented Oct 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIP++1: First pass. #1

BIP++1: First pass. #1

xanather commented Oct 15, 2022 •

edited

xanather commented Oct 15, 2022

xanather commented Oct 15, 2022

xanather commented Oct 15, 2022 •

edited

xanather commented Oct 15, 2022 •

edited

xanather commented Oct 15, 2022

xanather commented Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

xanather Oct 15, 2022

BIP++1: First pass. #1

Are you sure you want to change the base?

BIP++1: First pass. #1

Conversation

xanather commented Oct 15, 2022 • edited

xanather commented Oct 15, 2022

xanather commented Oct 15, 2022

xanather commented Oct 15, 2022 • edited

xanather commented Oct 15, 2022 • edited

xanather commented Oct 15, 2022

xanather commented Oct 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xanather commented Oct 15, 2022 •

edited

xanather commented Oct 15, 2022 •

edited

xanather commented Oct 15, 2022 •

edited