Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
luke-jr wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/blockencodings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@

#include <unordered_map>

#define MIN_TRANSACTION_BASE_SIZE (::GetSerializeSize(CTransaction(), SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS))

CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block, bool fUseWTXID) :
nonce(GetRand(std::numeric_limits<uint64_t>::max())),
shorttxids(block.vtx.size() - 1), prefilledtxn(1), header(block) {
Expand Down Expand Up @@ -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;
if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_BASE_SIZE)
if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_SIZE)
Copy link
Copy Markdown
Contributor

@TheBlueMatt TheBlueMatt Jan 24, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@luke-jr luke-jr Jan 25, 2017

Choose a reason for hiding this comment

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

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

return READ_STATUS_INVALID;

assert(header.IsNull() && txn_available.empty());
Expand Down
2 changes: 2 additions & 0 deletions src/consensus/consensus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ static const unsigned int MAX_BLOCK_WEIGHT = 4000000;
static const unsigned int MAX_BLOCK_BASE_SIZE = 1000000;
/** The maximum allowed number of signature check operations in a block (network rule) */
static const int64_t MAX_BLOCK_SIGOPS_COST = 80000;
/** Smallest possible transaction size */
static const unsigned int MIN_TRANSACTION_SIZE = 60;
Copy link
Copy Markdown
Member

@sipa sipa Jan 24, 2017

Choose a reason for hiding this comment

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

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

/** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */
static const int COINBASE_MATURITY = 100;

Expand Down
10 changes: 10 additions & 0 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,16 @@ bool InitSanityCheck(void)
if (!glibc_sanity_test() || !glibcxx_sanity_test())
return false;

CMutableTransaction tx;
tx.vin.resize(1);
tx.vout.resize(1);
size_t nMinTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
size_t nMinStrippedTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS);
if (MIN_TRANSACTION_SIZE != nMinTxSize || MIN_TRANSACTION_SIZE != nMinStrippedTxSize) {
InitError(strprintf("MIN_TRANSACTION_SIZE verification failure: const %u vs min=%u vs minstripped=%u", MIN_TRANSACTION_SIZE, nMinTxSize, nMinStrippedTxSize));
return false;
}
Copy link
Copy Markdown
Contributor

@dcousens dcousens Jan 24, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@jnewbery jnewbery Jan 24, 2017

Choose a reason for hiding this comment

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

+1 for a comment here.


return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/merkleblock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ uint256 CPartialMerkleTree::ExtractMatches(std::vector<uint256> &vMatch, std::ve
if (nTransactions == 0)
return uint256();
// check for excessively high numbers of transactions
if (nTransactions > MAX_BLOCK_BASE_SIZE / 60) // 60 is the lower bound for the size of a serialized CTransaction
if (nTransactions > MAX_BLOCK_BASE_SIZE / MIN_TRANSACTION_SIZE)
return uint256();
// there can never be more hashes provided than one for every txid
if (vHash.size() > nTransactions)
Expand Down