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

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2018/08/nChainTx changing 1 files +1 −1
  1. Sjors commented at 2:58 pm on August 4, 2018: member

    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:

     04 bytes version
 1    1 byte input count
 2        36 bytes outpoint
 3        1 byte scriptSigLen (0x00)
 4        0 bytes scriptSig
 5        4 bytes sequence
 6    1 byte output count
 7        8 bytes value
 8        1 byte scriptPubKeyLen
 9        1 byte scriptPubKey (OP_TRUE)
10    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.

  2. fanquake added the label Docs on Aug 4, 2018
  3. Sjors commented at 4:08 pm on August 4, 2018: member
    Something like this, plus some release notes, should do? https://github.com/bitcoin/bitcoin/compare/master...Sjors:2018/08/nChainTx-64
  4. MarcoFalke commented at 4:18 pm on August 4, 2018: member
    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)
  5. Sjors commented at 5:06 pm on August 4, 2018: member

    @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.

  6. Sjors commented at 6:55 pm on August 4, 2018: member

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

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

    Alternatively, even simpler:

    1. 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: https://github.com/bitcoin/bitcoin/pull/1677/commits/857c61df0b71c8a0482b1bf8fc55849f8ad831b8#diff-e8db9b851adc2422aadfffca88f14c91R1371

  7. gmaxwell commented at 7:28 pm on August 4, 2018: contributor

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

  8. Sjors commented at 7:38 pm on August 4, 2018: member
    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)
  9. sipa commented at 8:23 pm on August 4, 2018: member

    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.

  10. gmaxwell commented at 0:44 am on August 5, 2018: contributor
    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. :)
  11. Sjors commented at 11:55 am on November 30, 2018: member
    Anyway, the comment change itself is correct, right? (ignoring the other branch where I suggest a fix)
  12. MarcoFalke commented at 8:49 pm on December 22, 2018: member
    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?
  13. practicalswift commented at 10:11 am on March 21, 2019: contributor

    utACK ca5ed1b2462520ca2df598cb2af115028534587c

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

  14. jamesob commented at 2:40 pm on March 21, 2019: member

    utACK https://github.com/bitcoin/bitcoin/pull/13875/commits/ca5ed1b2462520ca2df598cb2af115028534587c

    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.

  15. in src/chain.h:172 in ca5ed1b246 outdated 
    199@@ -200,7 +200,7 @@ class CBlockIndex
200 
201     //! (memory only) Number of transactions in the chain up to and including this block.
202     //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
203-    //! Change to 64-bit type when necessary; won't happen before 2030
204+    //! Change to 64-bit type before 2024-2030, depending on SegWit adoption.
    MarcoFalke commented at 2:57 pm on March 21, 2019:

    I’d rather just put the worst-worst case here, which is 60 bytes per tx. Since it seems unlikely in the first place that everyone would be using 1-in-1-out txs with just OP_TRUE as witness.

     0    4 bytes version
 1    1 byte input count
 2        36 bytes outpoint
 3        1 byte scriptSigLen (0x00)
 4        0 bytes scriptSig
 5        4 bytes sequence
 6    1 byte output count
 7        8 bytes value
 8        1 byte scriptPubKeyLen
 9        1 byte scriptPubKey (OP_TRUE)
10    4 bytes locktime
    Sjors commented at 4:12 pm on October 21, 2019:
    Rebased, and updated PR description and source comment.
  16. DrahtBot closed this on Apr 28, 2019
  17. DrahtBot reopened this on Apr 28, 2019
  18. DrahtBot commented at 8:59 pm on October 16, 2019: member

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

    Conflicts

    No conflicts as of last run.

  19. DrahtBot added the label Needs rebase on Oct 17, 2019
  20. Sjors force-pushed on Oct 21, 2019
  21. Sjors force-pushed on Oct 21, 2019
  22. DrahtBot removed the label Needs rebase on Oct 21, 2019
  23. MarcoFalke deleted a comment on Oct 22, 2019
  24. laanwj referenced this in commit a25945318f on Oct 28, 2019
  25. in src/chain.h:173 in 8dc90f1449 outdated 
    168@@ -169,7 +169,7 @@ class CBlockIndex
169 
170     //! (memory only) Number of transactions in the chain up to and including this block.
171     //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
172-    //! Change to 64-bit type when necessary; won't happen before 2030
173+    //! Change to 64-bit type before 2024 (assuming worst case of 60 byte transactions).
    theStack commented at 0:40 am on January 24, 2020:

    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.

    gmaxwell commented at 6:55 pm on January 24, 2020:
    Yes? A transaction can have both an empty scriptsig and an emptyscriptpubkey. It isn’t spending itself, after all.
    theStack commented at 7:19 pm on January 24, 2020:
    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.
  26. Sjors requested review from MarcoFalke on May 4, 2020
  27. Sjors force-pushed on Jun 10, 2020
  28. instagibbs commented at 2:53 am on December 24, 2020: member

    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

  29. DrahtBot added the label Needs rebase on Feb 16, 2021
  30. Sjors force-pushed on Feb 17, 2021
  31. Sjors commented at 3:34 pm on February 17, 2021: member
    Rebased after #19806
  32. doc: nChainTx needs to become a 64-bit earlier due to SegWit ef72e9bd41
  33. Sjors force-pushed on Feb 17, 2021
  34. DrahtBot removed the label Needs rebase on Feb 17, 2021
  35. practicalswift commented at 9:41 pm on March 20, 2021: contributor
    re-ACK ef72e9bd4124645fe2d00521a71c1c298d760225
  36. jarolrod commented at 11:38 pm on April 6, 2021: member

    ACK ef72e9bd4124645fe2d00521a71c1c298d760225

    looks correct

  37. theStack approved
  38. theStack commented at 5:16 pm on October 14, 2021: member
    ACK ef72e9bd4124645fe2d00521a71c1c298d760225
  39. laanwj deleted a comment on Oct 20, 2021
  40. laanwj merged this on Oct 20, 2021
  41. laanwj closed this on Oct 20, 2021
  42. Sjors deleted the branch on Oct 20, 2021
  43. sidhujag referenced this in commit d11a7497a7 on Oct 20, 2021
  44. vijaydasmp referenced this in commit 694e3d28e0 on Mar 26, 2022
  45. vijaydasmp referenced this in commit e3a0d0c1da on Mar 26, 2022
  46. vijaydasmp referenced this in commit 22bccd4885 on Mar 27, 2022
  47. vijaydasmp referenced this in commit 98747a8eb0 on Mar 28, 2022
  48. vijaydasmp referenced this in commit bc71c65f44 on Mar 31, 2022
  49. vijaydasmp referenced this in commit dca1c35db4 on Apr 2, 2022
  50. vijaydasmp referenced this in commit f946ed044d on Apr 2, 2022
  51. vijaydasmp referenced this in commit 8a03aaa5c0 on Apr 15, 2022
  52. DrahtBot locked this on Oct 30, 2022


Sjors MarcoFalke gmaxwell sipa practicalswift jamesob DrahtBot theStack instagibbs jarolrod


MarcoFalke

Labels
Docs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me