Define, check, and use MIN_TRANSACTION

luke-jr commented at 4:05 AM on January 24, 2017: member

Define, check, and use MIN_TRANSACTION_SIZE as a const

Two places use the minimal transaction size to calculate a maximum transaction count. For compact blocks, the value used is actually incorrect: 10 (no inputs/outputs) instead of 60 (at least one input and output is required), because it is calculated on demand based on the CTransaction() constructor.

This commit defines a const with the correct value (60), and uses it in both locations. To ensure the value is always correct, InitSanityCheck makes sure at startup that it in fact matches the size of such a minimal transaction.

Define, check, and use MIN_TRANSACTION_SIZE as a const

Two places use the minimal transaction size to calculate a maximum transaction count.
For compact blocks, the value used is actually incorrect: 10 (no inputs/outputs) instead of 60 (at least one input and output is required), because it is calculated on demand based on the CTransaction() constructor.

This commit defines a const with the correct value (60), and uses it in both locations.
To ensure the value is always correct, InitSanityCheck makes sure at startup that it in fact matches the size of such a minimal transaction.

db626ef05a

in src/init.cpp:None in db626ef05a

 699 | +    size_t nMinTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
 700 | +    size_t nMinStrippedTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS);
 701 | +    if (MIN_TRANSACTION_SIZE != nMinTxSize || MIN_TRANSACTION_SIZE != nMinStrippedTxSize) {
 702 | +        InitError(strprintf("MIN_TRANSACTION_SIZE verification failure: const %u vs min=%u vs minstripped=%u", MIN_TRANSACTION_SIZE, nMinTxSize, nMinStrippedTxSize));
 703 | +        return false;
 704 | +    }

dcousens commented at 4:07 AM on January 24, 2017:

For clarity, why not put this in an individual function called here? Or at least comment as to what this mini 'code-block' is for?

jnewbery commented at 2:08 PM on January 24, 2017:

+1 for a comment here.

dcousens approved

in src/consensus/consensus.h:None in db626ef05a

  15 | @@ -16,6 +16,8 @@ static const unsigned int MAX_BLOCK_WEIGHT = 4000000;
  16 |  static const unsigned int MAX_BLOCK_BASE_SIZE = 1000000;
  17 |  /** The maximum allowed number of signature check operations in a block (network rule) */
  18 |  static const int64_t MAX_BLOCK_SIGOPS_COST = 80000;
  19 | +/** Smallest possible transaction size */
  20 | +static const unsigned int MIN_TRANSACTION_SIZE = 60;

sipa commented at 4:40 AM on January 24, 2017:

Maybe add a comment to explain that this is not an independent consensus rule, but just the shortest otherwise valid serialized transaction?

JeremyRubin commented at 5:21 AM on January 24, 2017: contributor

Concept ACK. Having quantities like this defined centrally would be a boon for some of the things that I have worked on.

I have a latent PR that defines code similar to this for a few other related quantities (but not specifically min_tx_size). I could push up just the constants commit if desired. However, I determined that these are more appropriately placed in validation.h than consensus.h for reasons similar to what @sipa has said above.

jonasschnelli added the label Refactoring on Jan 24, 2017

jonasschnelli commented at 8:03 AM on January 24, 2017: contributor

Concept ACK

jnewbery commented at 2:08 PM on January 24, 2017: member

ut ACK

in src/blockencodings.cpp:None in db626ef05a

  47 | @@ -50,7 +48,7 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const uint256& txhash) const {
  48 |  ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) {
  49 |      if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty()))
  50 |          return READ_STATUS_INVALID;
  51 | -    if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_BASE_SIZE)
  52 | +    if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_SIZE)

TheBlueMatt commented at 11:19 PM on January 24, 2017:

This is technically incorrect...someone relaying you a valid-to-deserialize and valid-work block which contained a 10-byte transaction would get banned, despite the BIP pretty clearly indicating they should be allowed to do so.

luke-jr commented at 5:56 AM on January 25, 2017:

Hmm. So compact blocks effectively force a specific deserialisation implementation into a consensus-critical status? (Or was it already for some reason I'm not thinking of?)

TheBlueMatt commented at 1:37 PM on January 25, 2017: member

Compact blocks sets specific rules for things which are valid to relay, even if invalid in blocks.

The spec says "As high-bandwidth mode permits relaying of CMPCTBLOCK messages prior to full validation (requiring only that the block header is valid before relay), nodes SHOULD NOT ban a peer for announcing a new block with a CMPCTBLOCK message that is invalid, but has a valid header."

Because of other parts of the spec, a node must be able to respond to GETBLOCKTXN messages with transactions which are in the block, but there is clear implication that you need not verify anything else. In fact, your implementation here, I believe isn't self-consistent - do we check transaction lengths before relay using NewPoWValidNlock?

On January 25, 2017 12:56:29 AM EST, Luke Dashjr notifications@github.com wrote:

luke-jr commented on this pull request.

if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size()

if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size()

MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_SIZE)

MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_BASE_SIZE)

@@ -50,7 +48,7 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const uint256& txhash) const { ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<uint256, CTransactionRef>>& extra_txn) { if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty())) return READ_STATUS_INVALID;

Hmm. So compact blocks effectively force a specific deserialisation implementation into a consensus-critical status? (Or was it already for some reason I'm not thinking of?)

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9621

luke-jr commented at 11:16 PM on February 1, 2017: member

Okay, this is more complex than anticipated then. Closing for now.

luke-jr closed this on Feb 1, 2017

DrahtBot locked this on Sep 8, 2021

Define, check, and use MIN_TRANSACTION_SIZE as a const #9621