[doc] nChainTx needs to become a 64-bit earlier due to SegWit #13875

Sjors · 2018-08-04T14:58:30Z

As of block 597,379 txcount is 460,596,047 (see chainparams.cpp), while uint32 can handle up to 4,294,967,296.

Pre segwit the minimum transaction size was 166 bytes, so the worst case number of transactions per block was ~6000. As the original source comment for unsigned int nChainTx says, that should last until the year 2030.

With SegWit the smallest possible transaction is 60 bytes (potentially increased to 65 with a future soft fork, see #15482), without a witness:

4 bytes version
    1 byte input count
        36 bytes outpoint
        1 byte scriptSigLen (0x00)
        0 bytes scriptSig
        4 bytes sequence
    1 byte output count
        8 bytes value
        1 byte scriptPubKeyLen
        1 byte scriptPubKey (OP_TRUE)
    4 bytes locktime

That puts the maximum number of transactions per block at 16,666 so we might have to deal with this as early as a block 827,450 in early 2024.

Given that it's a memory-only thing and we want to allow users many years to upgrade, I would suggest fixing this in v0.20 and back-porting it.

Sjors · 2018-08-04T16:08:17Z

Something like this, plus some release notes, should do? master...Sjors:2018/08/nChainTx-64

maflcko · 2018-08-04T16:18:22Z

Could also rename both to m_num_tx according to release notes? (That would also help with review, since each usage of the member would have to be checked anyway)

Sjors · 2018-08-04T17:06:44Z

@MarcoFalke done (I updated the link above)

I used m_chain_num_tx as distinct from nTx which can later be renamed to m_num_tx. That rename surfaced a bunch of things I hadn't thought about, which should be better now.

Sjors · 2018-08-04T18:55:13Z

If it's still necessary by that time, the following soft fork should prevent any issues for existing clients (unless I missed something):

The block which reaches nChainTx == UINT32_MAX much reach exactly UINT32_MAX total transactions.
The block afer that must have nTx >= 2.
Repeat this every UINT32_MAX transactions if necessary.

Alternatively, even simpler:

nChainTx modulo UINT32_MAX + 1 may not be 0 for any future block

Both options prevent nChainTx from becoming 0 which is the only special case, at least as of current master.

For historical reference, this counter was added in 2012: 857c61d#diff-e8db9b851adc2422aadfffca88f14c91R1371

gmaxwell · 2018-08-04T19:28:05Z

Or we could eliminate the number, the only real use for it is progress estimation (vs just using it as a one bit flag to indicate if a block has been processed yet or not), and we could simply use a single shared counter and be slightly off due to reorgs (which shouldn't be happening during IBD regardless).

As an aside, segwit didn't change the size of the minimum workable transaction size, both before and after a transaction can be 60 bytes. (consider: otherwise segwit wouldn't be a soft fork if it allowed something that wasn't possible before...)

Sjors · 2018-08-04T19:38:36Z

I noticed it was used for progress measurement, which the above soft fork wouldn't fix, but that seemed unimportant. ~~I didn't realize it was only used for that; I assumed it was doing more given the many occurances, but hadn't digged into what yet.~~ (nvm; you didn't say that)

sipa · 2018-08-04T20:23:27Z

As far as I know it is used (a) for progress estimation and (b) being nonzero is used as proxy for "we have downloaded all blocks up to this point in the chain".

It certainly has no effect on consensus rules.

gmaxwell · 2018-08-05T00:44:37Z

We should change the non-progress part to be a flag. Separately, we might want to drop the field entirely just to reduce the size of the index, progress can be done in ways that don't involve megabytes of memory usage. :)

Sjors · 2018-11-30T11:55:10Z

Anyway, the comment change itself is correct, right? (ignoring the other branch where I suggest a fix)

maflcko · 2018-12-22T20:49:52Z

Making it a global counter would mean we'd have to keep both around for some grace period so that they could be stored in the database and allow downgrading the software?

practicalswift · 2019-03-21T10:11:14Z

utACK ca5ed1b

Adding a comment makes sense and can't hurt. I think we are ready for merge.

jamesob · 2019-03-21T14:40:21Z

utACK ca5ed1b

Agree that nChainTx is probably worth dropping longterm in lieu of a "do we have all blocks downloaded up to this point in the chain" flag since the only other usage is for progress logging. Memory savings on CBlockIndex are probably worth a slightly fuzzier indication of progress.

src/chain.h

DrahtBot · 2019-10-16T20:59:43Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

…yHeaderTip fa39809 Avoid unused call to GuessVerificationProgress in NotifyHeaderTip (MarcoFalke) Pull request description: `GuessVerificationProgress` for a header (not a block) is always 0 because the number of txs in the block can not be determined from the header alone. Anyway, this result was never used, so we can optimize this call by hardcoding 0. This is the next commit in a series of changes toward removing nChainTx (see #14863, #13875) ACKs for top commit: promag: Code review ACK fa39809, missed that. laanwj: ACK fa39809 Tree-SHA512: 11016f8dbb1af1cf75241948d1ad35eac0c79d1311cd0db8c6ec806df2a9e3dc5f998dbd66ccbad5d84564e6cec7fe21ce7a2a13c2b34c746e2d3b31aa1db53a

theStack · 2020-01-24T00:40:54Z

src/chain.h

Being really pedantic here, but I think the worst-case tx size is 61 bytes rather than 60:

Skeleton: Version (4 bytes) + In-Count (1 byte) + Out-Count (1 byte) + Lock-Time (4 bytes) => 10 bytes

Input: Outpoint (36 bytes) + scriptSigLen (1 byte) + SequenceNr (4 bytes) => 41 bytes

Output: Value (8 bytes) + scriptPubKeyLen (1 byte) + scriptPubKey (1 byte) => 10 bytes

A 60 byte tx would only be possible with both an empty scriptSig and an empty scriptPubKey.

Yes? A transaction can have both an empty scriptsig and an emptyscriptpubkey. It isn't spending itself, after all.

Okay, good to know. The minimum tx size breakdown in the PR description (as well as a comment by MarcoFalke) included a 1-byte scriptPubKey (OP_TRUE, i.e. anyonecanspend), which led me to the wrong conclusion that empty scriptPubKeys are not valid.

instagibbs · 2020-12-24T02:53:44Z

Agree that nChainTx is probably worth dropping longterm in lieu of a "do we have all blocks downloaded up to this point in the chain" flag since the only other usage is for progress logging.

It's actually used for verifytxoutproof to check the merkle proof depth is correct to avoid https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-February/016697.html

Sjors · 2021-02-17T15:34:56Z

Rebased after #19806

practicalswift · 2021-03-20T21:41:39Z

re-ACK ef72e9b

jarolrod

ACK ef72e9b

looks correct

theStack

ACK ef72e9b

…due to SegWit ef72e9b doc: nChainTx needs to become a 64-bit earlier due to SegWit (Sjors Provoost) Pull request description: As of block 597,379 txcount is 460,596,047 (see `chainparams.cpp`), while `uint32` can handle up to 4,294,967,296. Pre segwit the [minimum transaction size](https://en.bitcoin.it/wiki/Maximum_transaction_rate) was 166 bytes, so the worst case number of transactions per block was ~6000. As the original source comment for `unsigned int nChainTx` says, that should last until the year 2030. With SegWit the smallest possible transaction is 60 bytes (potentially increased to 65 with a future soft fork, see bitcoin#15482), without a witness: ``` 4 bytes version 1 byte input count 36 bytes outpoint 1 byte scriptSigLen (0x00) 0 bytes scriptSig 4 bytes sequence 1 byte output count 8 bytes value 1 byte scriptPubKeyLen 1 byte scriptPubKey (OP_TRUE) 4 bytes locktime ``` That puts the maximum number of transactions per block at 16,666 so we might have to deal with this as early as a block 827,450 in early 2024. Given that it's a memory-only thing and we want to allow users many years to upgrade, I would suggest fixing this in v0.20 and back-porting it. ACKs for top commit: practicalswift: re-ACK ef72e9b jarolrod: ACK ef72e9b theStack: ACK ef72e9b Tree-SHA512: d8509ba7641796cd82af156354ff3a12ff7ec0f7b11215edff6696e95f8ca0e3596f719f3492ac3acb4b0884ac4e5bddc76f107b656bc2ed95a8ef1b2b5d4f71

…n NotifyHeaderTip fa39809 Avoid unused call to GuessVerificationProgress in NotifyHeaderTip (MarcoFalke) Pull request description: `GuessVerificationProgress` for a header (not a block) is always 0 because the number of txs in the block can not be determined from the header alone. Anyway, this result was never used, so we can optimize this call by hardcoding 0. This is the next commit in a series of changes toward removing nChainTx (see bitcoin#14863, bitcoin#13875) ACKs for top commit: promag: Code review ACK fa39809, missed that. laanwj: ACK fa39809 Tree-SHA512: 11016f8dbb1af1cf75241948d1ad35eac0c79d1311cd0db8c6ec806df2a9e3dc5f998dbd66ccbad5d84564e6cec7fe21ce7a2a13c2b34c746e2d3b31aa1db53a

fanquake added the Docs label Aug 4, 2018

maflcko reviewed Mar 21, 2019

View reviewed changes

src/chain.h Outdated Show resolved Hide resolved

DrahtBot closed this Apr 28, 2019

DrahtBot reopened this Apr 28, 2019

DrahtBot added the Needs rebase label Oct 17, 2019

Sjors force-pushed the 2018/08/nChainTx branch 2 times, most recently from 65aea5a to 8dc90f1 Compare October 21, 2019 16:11

DrahtBot removed the Needs rebase label Oct 21, 2019

bitcoin deleted a comment from DrahtBot Oct 22, 2019

maflcko mentioned this pull request Oct 25, 2019

Avoid unused call to GuessVerificationProgress in NotifyHeaderTip #17250

Merged

theStack reviewed Jan 24, 2020

View reviewed changes

Sjors requested a review from maflcko May 4, 2020 19:33

Sjors force-pushed the 2018/08/nChainTx branch from 8dc90f1 to 5fac795 Compare June 10, 2020 13:39

DrahtBot mentioned this pull request Oct 28, 2020

validation: UTXO snapshot activation #19806

Merged

DrahtBot added the Needs rebase label Feb 16, 2021

Sjors force-pushed the 2018/08/nChainTx branch from 5fac795 to 1c5f2bf Compare February 17, 2021 15:34

doc: nChainTx needs to become a 64-bit earlier due to SegWit

ef72e9b

Sjors force-pushed the 2018/08/nChainTx branch from 1c5f2bf to ef72e9b Compare February 17, 2021 15:36

DrahtBot removed the Needs rebase label Feb 17, 2021

jarolrod reviewed Apr 6, 2021

View reviewed changes

theStack approved these changes Oct 14, 2021

View reviewed changes

bitcoin deleted a comment from xzavjierr Oct 20, 2021

laanwj merged commit c8e68b4 into bitcoin:master Oct 20, 2021

Sjors deleted the 2018/08/nChainTx branch October 20, 2021 15:16

bitcoin locked and limited conversation to collaborators Oct 30, 2022

[doc] nChainTx needs to become a 64-bit earlier due to SegWit #13875

[doc] nChainTx needs to become a 64-bit earlier due to SegWit #13875

Uh oh!

Conversation

Sjors commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 4, 2018

Uh oh!

Sjors commented Aug 4, 2018

Uh oh!

Sjors commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Aug 4, 2018

Uh oh!

gmaxwell commented Aug 5, 2018

Uh oh!

Sjors commented Nov 30, 2018

Uh oh!

maflcko commented Dec 22, 2018

Uh oh!

practicalswift commented Mar 21, 2019

Uh oh!

jamesob commented Mar 21, 2019

Uh oh!

Uh oh!

DrahtBot commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

gmaxwell Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

theStack Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Dec 24, 2020

Uh oh!

Sjors commented Feb 17, 2021

Uh oh!

practicalswift commented Mar 20, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Sjors commented Aug 4, 2018 •

edited

Loading

Sjors commented Aug 4, 2018 •

edited

Loading

Sjors commented Aug 4, 2018 •

edited

Loading

gmaxwell commented Aug 4, 2018 •

edited

Loading

Sjors commented Aug 4, 2018 •

edited

Loading

DrahtBot commented Oct 16, 2019 •

edited

Loading