Introduce deploymentstatus #19438

pull ajtowns wants to merge 12 commits into bitcoin:master from ajtowns:202007-deployment-refactor changing 20 files +321 −192
  1. ajtowns commented at 5:03 PM on July 3, 2020: contributor

    Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from 11398 "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing".

    This provides three functions: DeploymentEnabled() which tests if a deployment can ever be active, DeploymentActiveAt() which checks if a deployment should be enforced in the given block, and DeploymentActiveAfter() which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.

    This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on cs_main. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.

  2. ajtowns added the label Refactoring on Jul 3, 2020
  3. ajtowns added the label Consensus on Jul 3, 2020
  4. DrahtBot commented at 7:35 PM on July 3, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. in src/consensus/params.h:14 in 5834cc8654 outdated
      10 | @@ -11,6 +11,15 @@
      11 |  
      12 |  namespace Consensus {
      13 |  
      14 | +enum BuriedDeployment
    


    JeremyRubin commented at 8:10 PM on July 3, 2020:

    prefer enum class to prevent leaking names.


    JeremyRubin commented at 8:10 PM on July 3, 2020:

    Also helps with avoiding overlap with SignalledDeployment.


    ajtowns commented at 9:14 PM on July 3, 2020:

    Leaking the names is the point -- otherwise you'd need to change every use from Consensus::DeploymentPos::SEGWIT to Consensus::BuriedDeployment::SEGWIT. With C++20, you will be able to specify enum class DeploymentPos { .. }; using enum DeploymentPos; but that's a while away.

  6. in src/consensus/params.h:21 in 5834cc8654 outdated
      15 | +{
      16 | +    // buried deployments get negative values to avoid overlap with SignalledDeployment
      17 | +    DEPLOYMENT_CLTV = -255,
      18 | +    DEPLOYMENT_DERSIG,
      19 | +    DEPLOYMENT_CSV,
      20 | +    DEPLOYMENT_SEGWIT,
    


    JeremyRubin commented at 8:11 PM on July 3, 2020:

    prefer explicitly numbering these.

    -255 is an extremely odd number, can you clarify why you picked it? It doesn't fit in a signed byte.


    ajtowns commented at 10:16 PM on July 3, 2020:

    Explicitly numbering them risks accidentally repeating a number. No particular objection to a different starting point. Could perhaps make it enum DeploymentPos : uint8_t and enum BuriedDeployment : int16_t { DEPLOYMENT_CLTV = 256, // larger than largest possible DeploymentPos value.

    (I guess 2**n is an "extremely even" number, so calling +/- 2**n-1 "extremely odd" makes a lot of sense...)

  7. in src/consensus/params.h:116 in 5834cc8654 outdated
      88 | @@ -80,7 +89,19 @@ struct Params {
      89 |      int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
      90 |      uint256 nMinimumChainWork;
      91 |      uint256 defaultAssumeValid;
      92 | +
      93 | +    inline int DeploymentHeight(BuriedDeployment dep) const
    


    JeremyRubin commented at 8:13 PM on July 3, 2020:

    Maybe tighter to make this a templated function as we're never calling DeploymentHeight with a variable, only literal.

    Then we get not just a warning, but a compiler error.


    ajtowns commented at 9:29 PM on July 3, 2020:

    I had tried that, but thought that it might be useful to be able to do for (dep = 0; dep < MAX_VERSIONBITS_DEPLOYMENTS; ++dep) { ... DeploymentActiveAt(pindex, consParams, dep) .. }. I think in all the cases where that currently comes up you want to know more fine grained details though. (Changing BuriedForkDescPushBack to take a BuriedDeployment instead of a height would make use of that, eg)

    I don't think the type checking is any better with the template though, as unfortunately you can invoke DeploymentHeight<(BuriedDeployment)9000>() just as easily as DeploymentHeight((BuriedDeployment)9000) with both g++ and clang++.

  8. in src/deploymentstatus.h:1 in 5834cc8654 outdated
       0 | @@ -0,0 +1,53 @@
       1 | +// Copyright (c) 2016-2019 The Bitcoin Core developers
    


    JeremyRubin commented at 8:15 PM on July 3, 2020:

    dates

  9. in src/deploymentstatus.h:17 in 5834cc8654 outdated
      15 | +
      16 | +/** Global cache for versionbits deployment status */
      17 | +extern VersionBitsCache versionbitscache GUARDED_BY(cs_main);
      18 | +
      19 | +/** Determine if a deployment is active for the next block */
      20 | +inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)
    


    JeremyRubin commented at 8:18 PM on July 3, 2020:

    This dispatch scares me a bit unless we use enum class -- enums could coerce poorly right?


    ajtowns commented at 9:38 PM on July 3, 2020:

    Nope; int (and the like) won't automatically coerce to enum, so with int x = Consensus::DEPLOYMENT_SEGWIT; return DeploymentActiveAt(pindex, params, x) you get errors:

    validation.cpp:1921:12: error: no matching function for call to 'DeploymentActiveAt'
        return DeploymentActiveAt(pindex, consensusparams, x);
               ^~~~~~~~~~~~~~~~~~
    ./deploymentstatus.h:32:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::BuriedDeployment' for 3rd argument
    inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::BuriedDeployment dep)
                ^
    ./deploymentstatus.h:37:13: note: candidate function not viable: no known conversion from 'int' to 'Consensus::DeploymentPos' for 3rd argument
    inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::DeploymentPos dep) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
                ^
    

    (even if it did, which enum it coerced to is ambiguous between the two types, resulting in a failure like error: call to 'DeploymentActiveAt' is ambiguous)

  10. JeremyRubin dismissed
  11. JeremyRubin commented at 8:24 PM on July 3, 2020: contributor

    Concept ACK and some lite code review feedback.

  12. ajtowns force-pushed on Jul 3, 2020
  13. DrahtBot cross-referenced this on Jul 4, 2020 from issue refactor: Make CCheckQueue RAII-styled by hebasto
  14. DrahtBot cross-referenced this on Jul 4, 2020 from issue BIP-325: Signet [consensus] by kallewoof
  15. DrahtBot cross-referenced this on Jul 4, 2020 from issue Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa
  16. DrahtBot cross-referenced this on Jul 11, 2020 from issue Refactor mempool.dat to be extensible, and store missing info by luke-jr
  17. DrahtBot cross-referenced this on Jul 24, 2020 from issue Replace unused BIP 9 logic with draft BIP 8 by luke-jr
  18. DrahtBot cross-referenced this on Jul 29, 2020 from issue [RFC] Package-relay: sender-initiated by ariard
  19. DrahtBot cross-referenced this on Aug 2, 2020 from issue assumeutxo by jamesob
  20. DrahtBot cross-referenced this on Aug 7, 2020 from issue Switch BlockMap to use an unordered_set under the hood by JeremyRubin
  21. DrahtBot cross-referenced this on Aug 20, 2020 from issue Net processing: move ProcessMessage() to PeerLogicValidation by jnewbery
  22. DrahtBot cross-referenced this on Aug 20, 2020 from issue refactor: Keep mempool interface in validation by MarcoFalke
  23. DrahtBot cross-referenced this on Aug 28, 2020 from issue [net processing] Move Misbehaving() to PeerManager by jnewbery
  24. DrahtBot added the label Needs rebase on Sep 7, 2020
  25. ajtowns force-pushed on Sep 8, 2020
  26. DrahtBot removed the label Needs rebase on Sep 8, 2020
  27. DrahtBot added the label Needs rebase on Sep 8, 2020
  28. ajtowns force-pushed on Sep 8, 2020
  29. DrahtBot removed the label Needs rebase on Sep 8, 2020
  30. DrahtBot cross-referenced this on Sep 19, 2020 from issue Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa
  31. DrahtBot cross-referenced this on Sep 19, 2020 from issue signet mining utility by ajtowns
  32. DrahtBot added the label Needs rebase on Sep 21, 2020
  33. ajtowns force-pushed on Sep 22, 2020
  34. DrahtBot removed the label Needs rebase on Sep 22, 2020
  35. DrahtBot cross-referenced this on Sep 22, 2020 from issue validation: re-delegate absurd fee checking from mempool to clients by glozow
  36. DrahtBot cross-referenced this on Sep 23, 2020 from issue History for Taproot PR #19953 by sipa
  37. DrahtBot cross-referenced this on Sep 26, 2020 from issue validation/util: add GetTransactionFee by glozow
  38. DrahtBot added the label Needs rebase on Oct 15, 2020
  39. ajtowns force-pushed on Oct 17, 2020
  40. DrahtBot removed the label Needs rebase on Oct 17, 2020
  41. DrahtBot cross-referenced this on Oct 17, 2020 from issue tree-wide: De-globalize ChainstateManager by dongcarl
  42. DrahtBot cross-referenced this on Nov 19, 2020 from issue rpc: deprecate `addresses` and `reqSigs` from rpc outputs by mjdietzx
  43. DrahtBot cross-referenced this on Nov 20, 2020 from issue refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size by theStack
  44. DrahtBot cross-referenced this on Dec 22, 2020 from issue [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl
  45. DrahtBot cross-referenced this on Dec 22, 2020 from issue [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex by dongcarl
  46. DrahtBot cross-referenced this on Dec 23, 2020 from issue net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns
  47. DrahtBot cross-referenced this on Dec 23, 2020 from issue rpc: Allow to ignore specific policy reject reasons by MarcoFalke
  48. in src/deploymentstatus.h:14 in 60de491a03 outdated
       9 | +#include <sync.h>
      10 | +#include <versionbits.h>
      11 | +
      12 | +#include <limits>
      13 | +
      14 | +extern RecursiveMutex cs_main;
    


    sipa commented at 5:00 AM on December 27, 2020:

    Any chance of just introducing a new lock for the versionbitscache, to avoid this semantically-cyclic dependency between validation and deploymentstatus?


    ajtowns commented at 12:45 AM on December 30, 2020:

    Yeah, can make VersionBitsCache be natively threadsafe, rather than having the caller manage a mutex (which should help if it ever ends up having some sort of shared lock too). I've done that, though I've added the lock as the first commit, since I think that makes the impact of the change clearer.

  49. sipa commented at 5:01 AM on December 27, 2020: member

    utACK 60de491a030011f3caeb42f0b650a1028905c54d

  50. DrahtBot cross-referenced this on Dec 29, 2020 from issue net processing: Only support version 2 compact blocks by jnewbery
  51. ajtowns force-pushed on Dec 30, 2020
  52. ajtowns force-pushed on Dec 30, 2020
  53. in src/deploymentstatus.h:24 in 318370aa0e outdated
      19 | +inline bool DeploymentActiveAt(const CBlockIndex* pindex, const Consensus::Params& params, Consensus::BuriedDeployment dep)
      20 | +{
      21 | +    return pindex->nHeight >= params.DeploymentHeight(dep);
      22 | +}
      23 | +
      24 | +/** Determine if a deployment is enabled (can ever be active */
    


    sipa commented at 7:16 AM on December 30, 2020:

    In commit "[refactor] Add deploymentstatus.h":

    YUUUUGE nit: missing parenthesis in comment

  54. in src/validation.cpp:3451 in 318370aa0e outdated
    3447 | @@ -3461,13 +3448,13 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
    3448 |   *  in ConnectBlock().
    3449 |   *  Note that -reindex-chainstate skips the validation that happens here!
    3450 |   */
    3451 | -static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
    3452 | +static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    


    sipa commented at 7:20 AM on December 30, 2020:

    In commit "[refactor] Add deploymentstatus.h":

    Is this "EXCLUSIVE_LOCKS_REQUIRED(cs_main)" still needed?


    ajtowns commented at 7:58 AM on January 2, 2021:

    No, don't think so. Removed.

  55. in src/deploymentstatus.h:24 in a323cdf30e outdated
      19 |      return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
      20 |  }
      21 |  
      22 | +inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
      23 | +{
      24 | +    return ThresholdState::ACTIVE == VersionBitsState(pindexPrev, params, dep, versionbitscache);
    


    sipa commented at 7:25 AM on December 30, 2020:

    In commit "[refactor] Add versionbits deployments to deploymentstatus.h":

    Perhaps assert that dep >= 0 in these functions (and dep < 0 for the buried ones)?


    ajtowns commented at 7:58 AM on January 2, 2021:

    I've added a ValidDeployment() function for checking this with a bit more precision.

    Might be worth revisiting @JeremyRubin's suggestion of using templated functions so it can be a static assertion that's resolved at compile-time? Having looked at it again, I think the only thing that would really end up making awkward is the rpc/blockchain.cpp stuff (ie, it would mean templating the SoftForkPushBack function and having the compiler duplicate the code and possibly not de-duplicate it when optimising) but that's not necessarily a big deal.

    I've added a draft commit that switches to templated functions and static_asserts to see what that's like. It does make rpc/blockchain a bit bigger (after compiling with clang) so might need be worth some tweaking to avoid that, and the syntax is a bit clunky, but it seems plausible. Thoughts?


    ajtowns commented at 3:42 AM on January 11, 2021:

    Okay, dropped the templated approach as too unwieldy for not enough benefit, so think this is resolved via ValidDeployment().

  56. sipa commented at 7:28 AM on December 30, 2020: member

    utACK 545930482da83953829308c131f173504374c569

    You may want to update the PR description regarding the locking change.

  57. sipa commented at 8:55 PM on December 30, 2020: member

    @JeremyRubin Have your comments been addressed?

  58. DrahtBot cross-referenced this on Dec 31, 2020 from issue scripted-diff: Bump copyright headers by MarcoFalke
  59. DrahtBot added the label Needs rebase on Dec 31, 2020
  60. ajtowns force-pushed on Jan 2, 2021
  61. ajtowns force-pushed on Jan 2, 2021
  62. ajtowns force-pushed on Jan 2, 2021
  63. ajtowns commented at 8:07 AM on January 2, 2021: contributor

    Rebased to after the copyright bump, addressed comments, added a draft commit that switches from runtime to static checks.

  64. DrahtBot removed the label Needs rebase on Jan 2, 2021
  65. ajtowns force-pushed on Jan 11, 2021
  66. ajtowns commented at 3:41 AM on January 11, 2021: contributor

    Dropped draft commit that made the ValidDeployment checks static_asserts.

  67. DrahtBot cross-referenced this on Jan 24, 2021 from issue rpc: faster JSON generation by martinus
  68. DrahtBot cross-referenced this on Jan 26, 2021 from issue Remove RewindBlockIndex logic by dhruv
  69. DrahtBot added the label Needs rebase on Feb 1, 2021
  70. ajtowns force-pushed on Feb 2, 2021
  71. ajtowns commented at 9:11 AM on February 2, 2021: contributor

    Rebased on top of #20749 due to conflict from both PRs changing nearby sections of validation.h

  72. DrahtBot removed the label Needs rebase on Feb 2, 2021
  73. DrahtBot cross-referenced this on Feb 2, 2021 from issue [Bundle 3/n] Prune remaining g_chainman usage in validation functions by dongcarl
  74. DrahtBot cross-referenced this on Feb 6, 2021 from issue Default to NODE_WITNESS in nLocalServices by dhruv
  75. DrahtBot cross-referenced this on Feb 11, 2021 from issue validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching by glozow
  76. DrahtBot cross-referenced this on Feb 12, 2021 from issue rpc/validation: enable packages through testmempoolaccept by glozow
  77. DrahtBot added the label Needs rebase on Feb 18, 2021
  78. ajtowns force-pushed on Feb 25, 2021
  79. DrahtBot removed the label Needs rebase on Feb 25, 2021
  80. DrahtBot cross-referenced this on Feb 25, 2021 from issue [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules by dongcarl
  81. DrahtBot added the label Needs rebase on Mar 4, 2021
  82. ajtowns force-pushed on Mar 4, 2021
  83. DrahtBot removed the label Needs rebase on Mar 4, 2021
  84. ajtowns cross-referenced this on Mar 6, 2021 from issue Convert taproot to flag day activation by ajtowns
  85. DrahtBot cross-referenced this on Mar 7, 2021 from issue tests: Add fuzzing harness for versionbits by ajtowns
  86. in src/deploymentstatus.cpp:17 in 073ae5b33c outdated
      12 | +/* Basic sanity checking for BuriedDeployment/DeploymentPos enums and
      13 | + * ValidDeployment check */
      14 | +
      15 | +static_assert(ValidDeployment(Consensus::DEPLOYMENT_TESTDUMMY), "sanity check of DeploymentPos failed (TESTDUMMY not valid)");
      16 | +static_assert(!ValidDeployment(Consensus::MAX_VERSION_BITS_DEPLOYMENTS), "sanity check of DeploymentPos failed (MAX value considered valid)");
      17 | +static_assert(!ValidDeployment(static_cast<Consensus::BuriedDeployment>(Consensus::DEPLOYMENT_TESTDUMMY)), "sanity check of BuridedDeployment failed (overlaps with DeploymentPos)");
    


    amitiuttarwar commented at 1:15 AM on March 9, 2021:

    typo: BuridedDeployment

  87. DrahtBot cross-referenced this on Mar 9, 2021 from issue BIP 341: Add Speedy Trial activation parameters by achow101
  88. DrahtBot cross-referenced this on Mar 9, 2021 from issue Implement BIP 8 based Speedy Trial activation by achow101
  89. DrahtBot cross-referenced this on Mar 9, 2021 from issue [Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl
  90. DrahtBot cross-referenced this on Mar 9, 2021 from issue Speedy trial support for versionbits by ajtowns
  91. jnewbery commented at 6:22 PM on March 9, 2021: member

    Concept ACK. I really like that deployments can be changed from versionbits to buried without any changes to the code in validation and other places.

  92. DrahtBot cross-referenced this on Mar 10, 2021 from issue Refactor versionbits deployments to avoid potential uninitialized variables by achow101
  93. DrahtBot cross-referenced this on Mar 10, 2021 from issue Genericide BIP9 in variable/type names and comments by luke-jr
  94. in src/consensus/params.h:16 in 3a32ec4a05 outdated
      10 | @@ -11,13 +11,25 @@
      11 |  
      12 |  namespace Consensus {
      13 |  
      14 | -enum DeploymentPos
      15 | +enum BuriedDeployment : int16_t
      16 | +{
      17 | +    // buried deployments get negative values to avoid overlap with SignalledDeployment
    


    amitiuttarwar commented at 10:01 PM on March 10, 2021:

    SignalledDeployment == DeploymentPos?

    although, I'd probably be down for a rename. I've been wondering what Pos stands for? position?


    ajtowns commented at 12:02 AM on March 11, 2021:

    "position in the vDeployments" array, I presume. Yeah, I think I changed it to "SignalledDeployment" at some point then dropped it as being too annoying to keep rebasing

  95. in src/consensus/params.h:29 in 3a32ec4a05 outdated
      25 | +
      26 | +enum DeploymentPos : uint16_t
      27 |  {
      28 |      DEPLOYMENT_TESTDUMMY,
      29 |      DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)
      30 |      // NOTE: Also add new deployments to VersionBitsDeploymentInfo in versionbits.cpp
    


    amitiuttarwar commented at 10:11 PM on March 10, 2021:

    I think this comment was previously incorrect (VersionBitsDeploymentInfo was in versionbitsinfo.cpp), but should definitely be updated since as of this PR it is defined in deploymentinfo.cpp

  96. amitiuttarwar commented at 10:13 PM on March 10, 2021: contributor

    just a couple doc notes for now. I've started reviewing, but it's going to take a while :)

  97. ajtowns commented at 4:09 AM on March 11, 2021: contributor

    Probably a good idea to review #21380 (Fuzzing harness for versionbits) before this one -- ~it has some refactors that will cause this to get rebased, and both the "speedy trial" implementations are based on it.~

  98. ajtowns force-pushed on Mar 11, 2021
  99. ajtowns commented at 6:21 AM on March 11, 2021: contributor

    Split deploymentinfo rename, DeploymentName and SoftForkPushBack into separate commits. (EDIT: and fix the comments amiti noted)

  100. ajtowns force-pushed on Mar 11, 2021
  101. DrahtBot cross-referenced this on Mar 13, 2021 from issue build: Add -Werror=implicit-fallthrough compile flag by hebasto
  102. in src/versionbits.h:78 in eb211ea0af outdated
      76 | - *  keyed by the bit position used to signal support. */
      77 | -struct VersionBitsCache
      78 | +/** BIP 9 allows multiple softforks to be deployed in parallel. We cache
      79 | + *  per-period state for every one of them. */
      80 | +
      81 | +class VersionBitsCache
    


    jnewbery commented at 10:08 AM on March 15, 2021:

    It seems a bit strange for this interface to be called 'cache'. I'd say this has a cache, not is a cache.


    ajtowns commented at 4:22 AM on March 17, 2021:

    I don't think I agree -- if there wasn't a cache, there would be no need for an object, these things would just be standalone functions, so calling the object a cache makes sense to me.

    There's some impedence mismatch though -- apart from having to have the object, the api is otherwise pretty independent of whether there's a cache or not.


    jnewbery commented at 9:23 AM on March 17, 2021:

    apart from having to have the object, the api is otherwise pretty independent of whether there's a cache or not.

    Indeed. The client doesn't care whether there's a cache or not (and for the static functions, there isn't even a cache). Generally, I think interfaces should be named according to the client usage, not the implementation specifics. Not a big deal, and shouldn't hold up this PR.


    ajtowns commented at 9:45 AM on March 17, 2021:

    The client does care a little about whether there's a cache -- without it, checking the state is potentially linear in the height of the block being tested, with it, it's amortized constant. Particularly for deployments with short periods (like bip91) that could be prohibitive.

    Perhaps it's worth considering renaming it to SignalledDeploymentManager or similar along with renaming from BIP9 to VBits or renaming DeploymentPos or a deeper refactoring of AbstractThresholdConditionChecker etc.


    jnewbery commented at 9:53 AM on March 17, 2021:

    I'd prefer a generic name like SignalledDeploymentManager, but it definitely shouldn't hold up this PR.


    jnewbery commented at 9:55 AM on March 17, 2021:

    The client does care a little about whether there's a cache

    Sorry, I meant in terms of correctness/observability. Of course a cache has implications for computational complexity.

  103. in src/versionbits.h:82 in eb211ea0af outdated
      81 | +class VersionBitsCache
      82 |  {
      83 | -    ThresholdConditionCache caches[Consensus::MAX_VERSION_BITS_DEPLOYMENTS];
      84 | +private:
      85 | +    Mutex cs;
      86 | +    ThresholdConditionCache caches[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] GUARDED_BY(cs);
    


    jnewbery commented at 10:12 AM on March 15, 2021:

    Couple of suggested changes:

    • use m_ naming convention (and cs -> mutex)
    • mark these mutable so the functions can all be const
        mutable Mutex m_mutex;
        mutable ThresholdConditionCache m_caches[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] GUARDED_BY(cs);
    

    ajtowns commented at 5:42 AM on March 17, 2021:

    Names changed; left as non-mutable and non-const.

  104. in src/versionbits.h:88 in eb211ea0af outdated
      87 | +
      88 | +public:
      89 | +    /** Get the numerical statistics for the BIP9 state for a given deployment as at pindexPrev. */
      90 | +    static BIP9Stats Statistics(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos);
      91 | +
      92 | +    static uint32_t Mask(const Consensus::Params& params, Consensus::DeploymentPos pos);
    


    jnewbery commented at 10:15 AM on March 15, 2021:

    What's the benefit of specifically these two functions being static? There will only ever be one instance of VersionBitsCache so theoretically everything here could be static. I'd suggest just dropping the static here.


    sipa commented at 11:32 PM on March 16, 2021:

    @jnewbery The class does have members variables, and the non-static member function do access them, so unless those variables become static too (which would be rather ugly), the other member functions can't be.

    I think it doesn't hurt to mark the function that don't need state static.


    ajtowns commented at 5:26 AM on March 17, 2021:

    I think this is the same impedence mismatch as "calling it a cache" -- they're static because they don't use the cache.

  105. in src/versionbits.cpp:220 in eb211ea0af outdated
     216 | +int32_t VersionBitsCache::ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params)
     217 | +{
     218 | +    LOCK(cs);
     219 | +    int32_t nVersion = VERSIONBITS_TOP_BITS;
     220 | +
     221 | +    for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {
    


    jnewbery commented at 10:44 AM on March 15, 2021:

    You can use the underlying type of the enum rather than casting it to an int:

        for (std::underlying_type<Consensus::DeploymentPos>::type i = 0; i < Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {
    

    ajtowns commented at 4:50 AM on March 17, 2021:

    We cast to an int to iterate in a few places, and changing them feels gratuitous -- we can't make the underlying type something that won't fit in an int because the fixed arrays we index into via the enum value would break too.


    jnewbery commented at 9:21 AM on March 17, 2021:

    ok

  106. in src/validation.cpp:3460 in eb211ea0af outdated
    3457 | @@ -3491,11 +3458,12 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
    3458 |  
    3459 |      // Reject outdated version blocks when 95% (75% on testnet) of the network has upgraded:
    3460 |      // check for version 2, 3 and 4 upgrades
    


    jnewbery commented at 10:49 AM on March 15, 2021:

    Comment can be removed. We're no longer interested in the activation method now that these deployments are buried.

  107. in src/validation.cpp:1876 in eb211ea0af outdated
    1875 | -    if (pindex->nHeight >= consensusparams.CSVHeight) {
    1876 | +    if (DeploymentActiveAt(pindex, consensusparams, Consensus::DEPLOYMENT_CSV)) {
    1877 |          flags |= SCRIPT_VERIFY_CHECKSEQUENCEVERIFY;
    1878 |      }
    1879 |  
    1880 |      // Start enforcing Taproot using versionbits logic.
    


    jnewbery commented at 10:50 AM on March 15, 2021:
        // Start enforcing Taproot.
    

    Actually, all of these comments can have s/Start enforcing/Enforce/.

  108. in src/deploymentstatus.cpp:7 in eb211ea0af outdated
       0 | @@ -0,0 +1,17 @@
       1 | +// Copyright (c) 2020 The Bitcoin Core developers
       2 | +// Distributed under the MIT software license, see the accompanying
       3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
       4 | +
       5 | +#include <deploymentstatus.h>
       6 | +#include <versionbits.h>
       7 | +
       8 | +#include <consensus/params.h>
    


    jnewbery commented at 11:27 AM on March 15, 2021:
    
    #include <consensus/params.h>
    #include <versionbits.h>
    
  109. in src/deploymentinfo.h:10 in eb211ea0af outdated
       5 | +#ifndef BITCOIN_DEPLOYMENTINFO_H
       6 | +#define BITCOIN_DEPLOYMENTINFO_H
       7 | +
       8 | +#include <consensus/params.h>
       9 | +
      10 | +#include <string.h>
    


    jnewbery commented at 11:32 AM on March 15, 2021:
    #include <string>
    

    (string.h is the c string library: https://en.cppreference.com/w/cpp/header/cstring)

  110. jnewbery commented at 11:46 AM on March 15, 2021: member

    ACK 58257b9a7ba841c1b569127bb05ca606780f853b

    A few minor suggestions inline.

  111. amitiuttarwar commented at 8:27 PM on March 16, 2021: contributor

    approach ACK, I like the strategy of using enums with safety checks + overloaded functions to abstract buried vs future deployments

  112. in src/versionbits.h:84 in 898d54177d outdated
      78 | @@ -79,11 +79,11 @@ struct VersionBitsCache
      79 |      void Clear();
      80 |  };
      81 |  
      82 | -/** Get the BIP9 state for a given deployment at the current tip. */
      83 | +/** Get the BIP9 state for a given deployment for the block after pindexPrev. */
      84 |  ThresholdState VersionBitsState(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos pos, VersionBitsCache& cache);
      85 | -/** Get the numerical statistics for the BIP9 state for a given deployment at the current tip. */
      86 | +/** Get the numerical statistics for the BIP9 state for a given deployment as at pindexPrev. */
    


    sipa commented at 11:04 PM on March 16, 2021:

    In "versionbits: correct doxygen comments"

    Nit: perhaps it's worth stopping using pindexPrev here if it's giving statistics for the specified block rather than its successor?


    ajtowns commented at 11:55 PM on March 16, 2021:

    It's giving the statistics for the retarget period that includes pindexPrev's successor, up to and including pindexPrev if pindexPrev is in the same retarget period, but not if pindexPrev was the last block in the previous retarget period. (That means you can't actually query the statistics for a fully completed retarget period -- you'll always get statistics for between 0 and 2015 blocks worth of signalling, never the full 2016 blocks. Or whatever the period is)

    I've been confused about this until recently, so not sure how to describe it well


    sipa commented at 4:29 AM on March 17, 2021:

    It could just say "Get statistics for the BIP9 state for the measuring window that inlcudes pindexPrev's successor." ?

    Feel free to disregard this.

  113. sipa commented at 11:35 PM on March 16, 2021: member

    Concept/approach/code review ACK; there are a bunch of nits by @jnewbery and @amitiuttarwar left that seem worth addressing.

  114. ajtowns force-pushed on Mar 17, 2021
  115. ajtowns commented at 5:41 AM on March 17, 2021: contributor

    Nits picked.

  116. jnewbery commented at 9:24 AM on March 17, 2021: member

    code review ACK e72e062e5a8279864d746776dc9072c112ddc014

  117. DrahtBot cross-referenced this on Mar 18, 2021 from issue Move external signer out of wallet module by Sjors
  118. in src/deploymentstatus.h:23 in e72e062e5a outdated
      18 | +{
      19 | +    assert(Consensus::ValidDeployment(dep));
      20 | +    return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
      21 | +}
      22 | +
      23 | +inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
    


    GeneFerneau commented at 2:22 AM on March 22, 2021:

    nit: With all these new functions taking a const CBlockIndex* ... argument, maybe use a const CBlockIndex& ... instead. Safer API, and can still use &pindexPrev to get a pointer (if needed).


    ajtowns commented at 3:38 AM on March 22, 2021:

    Could make sense to have DeploymentActiveAt(const CBlockIndex&, ...) but DeploymentActiveAfter can be called with nullptr, so can't be a reference.

  119. GeneFerneau commented at 2:22 AM on March 22, 2021: none

    code review ACK

  120. 0xB10C commented at 5:23 PM on March 22, 2021: contributor

    Concept ACK. Started reviewing

  121. DrahtBot cross-referenced this on Mar 22, 2021 from issue Implement BIP8 lockinontimeout by achow101
  122. in src/consensus/params.h:32 in 8b2aa89f9f outdated
      28 |      DEPLOYMENT_TESTDUMMY,
      29 |      DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)
      30 |      // NOTE: Also add new deployments to VersionBitsDeploymentInfo in versionbits.cpp
      31 |      MAX_VERSION_BITS_DEPLOYMENTS
      32 |  };
      33 | +constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep <= DEPLOYMENT_TAPROOT; }
    


    0xB10C commented at 11:44 AM on March 24, 2021:

    in 8b2aa89f9f6366cfd3a3dfccf2f7678740b4d0ea

    nit: couldn't this be < MAX_VERSION_BITS_DEPLOYMENTS to avoid changing this line when adding/burring a deployment?

    constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep < MAX_VERSION_BITS_DEPLOYMENTS; }
    
  123. DrahtBot cross-referenced this on Apr 3, 2021 from issue rpc, gui: bumpfee signer support by Sjors
  124. DrahtBot cross-referenced this on Apr 3, 2021 from issue refactor: Create blockstorage module by MarcoFalke
  125. in src/consensus/params.h:23 in 994b0ce58f outdated
      18 | +    DEPLOYMENT_CLTV,
      19 | +    DEPLOYMENT_DERSIG,
      20 | +    DEPLOYMENT_CSV,
      21 | +    DEPLOYMENT_SEGWIT,
      22 | +};
      23 | +constexpr bool ValidDeployment(BuriedDeployment dep) { return DEPLOYMENT_HEIGHTINCB <= dep && dep <= DEPLOYMENT_SEGWIT; }
    


    fjahr commented at 4:09 PM on April 3, 2021:

    very minor nit: If you add a DEPLOYMENT_BURIED_MAX and then use dep < DEPLOYMENT_BURIED_MAX instead of dep <= DEPLOYMENT_SEGWIT then this line wouldn't need to change when something is buried. But I am not sure if it's an improvement overall.


    ajtowns commented at 9:36 AM on April 16, 2021:

    Didn't want to add a DEPLOYMENT_BURIED_MAX since that would then need to be included in any switch statements.

  126. in src/deploymentstatus.h:20 in 994b0ce58f outdated
      11 | +
      12 | +/** Determine if a deployment is active for the next block */
      13 | +inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::BuriedDeployment dep)
      14 | +{
      15 | +    assert(Consensus::ValidDeployment(dep));
      16 | +    return (pindexPrev == nullptr ? 0 : pindexPrev->nHeight + 1) >= params.DeploymentHeight(dep);
    


    fjahr commented at 4:28 PM on April 3, 2021:

    nit: You could do !pindexPrev instead of == nullptr I think

  127. in src/consensus/params.h:130 in 994b0ce58f outdated
     114 | +        case DEPLOYMENT_CSV:
     115 | +            return CSVHeight;
     116 | +        case DEPLOYMENT_SEGWIT:
     117 | +            return SegwitHeight;
     118 | +        } // no default case, so the compiler can warn about missing cases
     119 | +        return std::numeric_limits<int>::max();
    


    fjahr commented at 4:34 PM on April 3, 2021:

    nit: Could use an optional as return value instead of this to signal if no height was found?


    ajtowns commented at 9:56 AM on April 5, 2021:

    That would mean checking and dereferencing the optional at every call site, which doesn't seem useful.


    MarcoFalke commented at 6:06 PM on July 1, 2021:

    Or maybe turn this dead code into an assert(false)? Crashing the node should be preferred over a consensus failure.

  128. in src/deploymentstatus.h:45 in 994b0ce58f outdated
      24 | +}
      25 | +
      26 | +/** Determine if a deployment is enabled (can ever be active) */
      27 | +inline bool DeploymentEnabled(const Consensus::Params& params, Consensus::BuriedDeployment dep)
      28 | +{
      29 | +    assert(Consensus::ValidDeployment(dep));
    


    fjahr commented at 4:36 PM on April 3, 2021:

    nit: This check is repeated three times only to protect DeploymentHeight() afaict, maybe move it into DeploymentHeight()?

  129. in src/versionbits.h:78 in e72e062e5a outdated
      71 | @@ -70,21 +72,32 @@ class AbstractThresholdConditionChecker {
      72 |      int GetStateSinceHeightFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const;
      73 |  };
      74 |  
      75 | -/** BIP 9 allows multiple softforks to be deployed in parallel. We cache per-period state for every one of them
      76 | - *  keyed by the bit position used to signal support. */
      77 | -struct VersionBitsCache
      78 | +/** BIP 9 allows multiple softforks to be deployed in parallel. We cache
      79 | + *  per-period state for every one of them. */
      80 | +
    


    fjahr commented at 4:59 PM on April 3, 2021:

    nit: accidental empty line?

  130. fjahr commented at 5:29 PM on April 3, 2021: contributor

    Code review ACK e72e062e5a8279864d746776dc9072c112ddc014

    Left a couple of nits, feel free to ignore unless you have to retouch.

  131. bitcoin deleted a comment on Apr 10, 2021
  132. bitcoin deleted a comment on Apr 10, 2021
  133. DrahtBot added the label Needs rebase on Apr 13, 2021
  134. ajtowns force-pushed on Apr 16, 2021
  135. DrahtBot removed the label Needs rebase on Apr 16, 2021
  136. ajtowns commented at 9:34 AM on April 16, 2021: contributor

    Rebased on top of #21666 (neighbouring line changed) and the ComputeBlockVersion tests in #21377. Changed DeploymentActiveAt() to take a reference to a block rather than a pointer, since nullptr isn't valid. Also split the move of ComputeBlockVersion into two steps, one for the move-only part and the other for the refactoring, since that seems a bit easier to review.

  137. DrahtBot cross-referenced this on Apr 16, 2021 from issue Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin
  138. DrahtBot cross-referenced this on Apr 16, 2021 from issue test: Check that no versionbits are re-used by MarcoFalke
  139. DrahtBot added the label Needs rebase on Apr 17, 2021
  140. ajtowns force-pushed on Apr 19, 2021
  141. ajtowns commented at 3:00 AM on April 19, 2021: contributor

    Rebased on top of #21391 due to ChainActive().Tip() changes.

    (Maybe consider reviewing conflicting pr #21691 first, since it's easy)

  142. ajtowns force-pushed on Apr 19, 2021
  143. DrahtBot removed the label Needs rebase on Apr 19, 2021
  144. DrahtBot cross-referenced this on Apr 19, 2021 from issue refactor: Move more stuff to blockstorage by MarcoFalke
  145. DrahtBot added the label Needs rebase on Apr 20, 2021
  146. ajtowns force-pushed on Apr 20, 2021
  147. DrahtBot removed the label Needs rebase on Apr 20, 2021
  148. ajtowns force-pushed on Apr 20, 2021
  149. in src/deploymentinfo.h:26 in 37458fdd10 outdated
      21 | +std::string DeploymentName(Consensus::BuriedDeployment dep);
      22 | +
      23 | +inline std::string DeploymentName(Consensus::DeploymentPos pos)
      24 | +{
      25 | +    assert(Consensus::ValidDeployment(pos));
      26 | +    return VersionBitsDeploymentInfo[pos].name;
    


    jnewbery commented at 11:39 AM on April 26, 2021:

    Consider using std::array and at() for bounds checking.


    ajtowns commented at 8:41 AM on April 30, 2021:

    Same could be said for vDeployments in Consensus::Params; think both are better left for some other refactoring.

  150. jnewbery commented at 12:19 PM on April 26, 2021: member

    utACK 37458fdd1033cb30ec0591d0932e7d6a6b0dee32

    I'm not convinced that a VersionBitsCache class is necessary. I think just as good would be to have a VersionBits namespace with free functions. The caching could all be hidden in the .cpp file, and only the functional interface would be exposed to client code.

  151. DrahtBot added the label Needs rebase on Apr 27, 2021
  152. ajtowns force-pushed on Apr 30, 2021
  153. ajtowns commented at 8:45 AM on April 30, 2021: contributor

    Rebased past #21009 ~(and added some comment updates to reflect that Rewind is gone now)~

  154. in src/chain.h:185 in b24e85139e outdated
     181 | @@ -182,7 +182,7 @@ class CBlockIndex
     182 |      //!
     183 |      //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
     184 |      //! load to avoid the block index being spuriously rewound.
     185 | -    //! @sa RewindBlockIndex
     186 | +    //! @sa NeedsRedownload
    


    MarcoFalke commented at 9:02 AM on April 30, 2021:

    How is this change different from fa49430914fd16fbc34078bd8929b6ef635a8c42 in #21789 ?


    ajtowns commented at 9:42 AM on April 30, 2021:

    :eyes: What change? There's no change here... No change at all...

  155. ajtowns force-pushed on Apr 30, 2021
  156. DrahtBot removed the label Needs rebase on Apr 30, 2021
  157. DrahtBot cross-referenced this on Apr 30, 2021 from issue refactor: Remove ::Params() global from CChainState by MarcoFalke
  158. DrahtBot added the label Needs rebase on May 5, 2021
  159. ajtowns force-pushed on May 6, 2021
  160. ajtowns commented at 2:43 PM on May 6, 2021: contributor

    Rebased past #21727.

    I'm not convinced that a VersionBitsCache class is necessary. @jnewbery - You're right it's not necessary; but I think minimising the number of globals and keeping the lock and the things the lock is guarding together (which then allows stronger locking assertions, see #20272 (comment)) is worthwhile enough. If you're implying the ComputeBlockVersion to versionbits.ComputeBlockVersion change is a bit ugly, that could be fixed just by adding an inline int32_t ComputeBlockVersion(pindexPrev, params) { return versionbits.ComputeBlockVersion(pindexPrev, params); } in deploymentstatus.h (where the versionbits global is declared).

  161. DrahtBot removed the label Needs rebase on May 6, 2021
  162. DrahtBot cross-referenced this on May 6, 2021 from issue [Bundle 7/7] validation: Farewell, global Chainstate! by dongcarl
  163. jnewbery commented at 7:47 AM on May 10, 2021: member

    See https://github.com/jnewbery/bitcoin/commit/000899c6753b69f141ffa07f7d2a12bd235824e3#diff-73b381667b1bb315180fc7e7a66992e79ad742972de5d0d2c1b8404d3d67e1b0 for the proposed changes in versionbits.h. The benefit is not exposing anything about the internal implementation outside of the .cpp file.

    I think minimising the number of globals and keeping the lock and the things the lock is guarding together (which then allows stronger locking assertions [...]

    I think it's fine to have a global with internal linkage in a single implementation file. The problems with globals all come when they're externally linked and accessed from many places in the code. The point about locking assertions also isn't relevant here since we're not using EXCLUSIVE_LOCKS_REQUIRED(!) or LOCKS_EXCLUDED() for this mutex.

  164. jnewbery commented at 7:53 AM on May 10, 2021: member

    utACK 63a6f6e5e695ca18c78a0dfb4d2261d1eb15ef4d.

    Verified git range-diff master 37458fdd10 HEAD. Only difference is resolving the merge conflict with #21009.

  165. laanwj added this to the "Blockers" column in a project

  166. DrahtBot cross-referenced this on Jun 2, 2021 from issue doc: Various validation doc fixups by MarcoFalke
  167. DrahtBot cross-referenced this on Jun 4, 2021 from issue net processing: Remove hash and fValidatedHeaders from QueuedBlock by jnewbery
  168. DrahtBot added the label Needs rebase on Jun 4, 2021
  169. ajtowns force-pushed on Jun 8, 2021
  170. ajtowns commented at 4:17 AM on June 8, 2021: contributor

    Rebased past #22121 ; split the last commit to reduce the ComputeBlockVersion to versionbits.ComputeBlockVersion noise.

  171. DrahtBot removed the label Needs rebase on Jun 8, 2021
  172. jnewbery commented at 8:45 AM on June 8, 2021: member

    Code review ACK 3696abb45e

    Verified the rebase using git range-diff.

  173. in src/consensus/params.h:115 in 8695986585 outdated
     110 | @@ -100,7 +111,25 @@ struct Params {
     111 |       */
     112 |      bool signet_blocks{false};
     113 |      std::vector<uint8_t> signet_challenge;
     114 | +
     115 | +    inline int DeploymentHeight(BuriedDeployment dep) const
    


    MarcoFalke commented at 12:31 PM on June 8, 2021:

    8695986585d8717a9ab484877d25325fbe78a3e8: The compiler already knows this is inline because the member function has a definition. Thus, the inline can be removed for brevity.

  174. in src/deploymentstatus.h:26 in 9773579504 outdated
      21 |  }
      22 |  
      23 | +inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
      24 | +{
      25 | +    assert(Consensus::ValidDeployment(dep));
      26 | +    return ThresholdState::ACTIVE == VersionBitsState(pindexPrev, params, dep, versionbitscache);
    


    MarcoFalke commented at 1:32 PM on June 8, 2021:

    97735795048b999acf5c79085ecdeb5df0413532: Any reason to not use ::versionbitscache? Especially since the g_ prefix is missing?

  175. in src/deploymentinfo.h:19 in 0529efe7ce outdated
      15 |      /** Whether GBT clients can safely ignore this rule in simplified usage */
      16 |      bool gbt_force;
      17 |  };
      18 |  
      19 | -extern const struct VBDeploymentInfo VersionBitsDeploymentInfo[];
      20 | +extern const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS];
    


    MarcoFalke commented at 1:36 PM on June 8, 2021:

    0529efe7ce918e564672a77da891d5e7a3c9657f: The compiler already knows that VBDeploymentInfo is a struct, so while touching this line it would be nice to remove this confusing extraneous keyword.

  176. in src/deploymentstatus.h:26 in 5eecf96666 outdated
      22 | @@ -23,7 +23,7 @@ inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus
      23 |  inline bool DeploymentActiveAfter(const CBlockIndex* pindexPrev, const Consensus::Params& params, Consensus::DeploymentPos dep)
      24 |  {
      25 |      assert(Consensus::ValidDeployment(dep));
      26 | -    return ThresholdState::ACTIVE == VersionBitsState(pindexPrev, params, dep, versionbitscache);
      27 | +    return ThresholdState::ACTIVE == versionbitscache.State(pindexPrev, params, dep);
    


    MarcoFalke commented at 1:46 PM on June 8, 2021:

    5eecf966664df05afb8bb8ea9d5a18c7f9d13acf: Maybe add the :: now, if editing previous commits would be too much rebase hassle?

  177. in src/miner.cpp:133 in 0967181ee8 outdated
     129 | @@ -130,7 +130,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     130 |      assert(pindexPrev != nullptr);
     131 |      nHeight = pindexPrev->nHeight + 1;
     132 |  
     133 | -    pblock->nVersion = ComputeBlockVersion(pindexPrev, chainparams.GetConsensus());
     134 | +    pblock->nVersion = versionbitscache.ComputeBlockVersion(pindexPrev, chainparams.GetConsensus());
    


    MarcoFalke commented at 1:54 PM on June 8, 2021:

    0967181ee84d6c0f1a5c07d6e366d2a48c548bde: :: :smile_cat:

  178. in src/test/versionbits_tests.cpp:412 in 3696abb45e outdated
     412 |          lastBlock = secondChain.Mine(min_activation_height, nTime, VERSIONBITS_LAST_OLD_BLOCK_VERSION).Tip();
     413 |      }
     414 |  
     415 |      // Check that we don't signal after activation
     416 | -    BOOST_CHECK_EQUAL(ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
     417 | +    BOOST_CHECK_EQUAL(versionbitscache.ComputeBlockVersion(lastBlock, params) & (1<<bit), 0);
    


    MarcoFalke commented at 2:03 PM on June 8, 2021:

    3696abb45e3d9b09d027fffd3fc9f19a15e21691: Could add :: and apply clang-format-diff to the touched lines?

  179. in src/rpc/blockchain.cpp:1346 in 32cf12a896 outdated
    1347 | @@ -1345,25 +1348,25 @@ static RPCHelpMan verifychain()
    1348 |      };
    1349 |  }
    1350 |  
    1351 | -static void BuriedForkDescPushBack(UniValue& softforks, const std::string &name, int softfork_height, int tip_height) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    


    MarcoFalke commented at 2:05 PM on June 8, 2021:

    32cf12a89619688182fcb175a63ba0356f170d9c: When removing the lock annotation here, maybe add a comment to LOCK(cs_main), that it must be held while all soft forks are pushed back even though there is no annotation requiring that. Otherwise the rpc might be racy.

    Alternatively keep the annotation?


    ajtowns commented at 1:56 AM on June 11, 2021:

    I don't think it should be racy? The versionbits structures are protected by VersionBitsCache::m_mutex and should be stable when provided a particular block, even if it's no longer the tip or a reorg is currently happening?


    MarcoFalke commented at 6:41 AM on June 11, 2021:

    Oh correct. active_chain_tip is kept unchanged while pushing back all soft forks.

  180. MarcoFalke approved
  181. MarcoFalke commented at 2:06 PM on June 8, 2021: member

    review ACK 3696abb45e3d9b09d027fffd3fc9f19a15e21691 🏌

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    review ACK 3696abb45e3d9b09d027fffd3fc9f19a15e21691 🏌
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUgjMAwAiCgBhWC+XZ97dymn6KaEOyw0mJa0Yd1hVK7XAO6dUZzAVdpIJ4LFzzxv
    K8wscByQ8ZbyM0jzaZYxj+KXNj0Mxc8nAooXKyL74myWD63Nc4yaXYLG5zDqAB3g
    guSg6JNAhb/efy1XTA/RhnVJ4q6n0LKhSxE4AuEl4X2m8QeDJ1nh5YYF+BYhx9gd
    mtbjEuWhWiUOVqI0vs2nEDp+UwR9K0saYIcXemPJJ0JKarQDcfyvTP1saxoRqc3S
    RJ++Y2QZy8tCAZnVIz3TAYfnJ+8TnTsKOnc/NUC5d0dpVxvt+bgNsUY5mV6EWU2o
    GNOnea8u55Va0l8QuU2A7jK7QXAdCKgT7jqi9+On3qnGINcGUpNvrGMvldNllBfz
    s6i/rqng6gAcHGdMofJ8MEKnZ2KpdE5G60CheXlNk0FDMBDW+iAehyQP5PfD2w4j
    hjJ9V5EH2PqibtGZlCB8H8Rf45FB2laTTBtWpejLJFPrtVor9zOeV2vxFhOSWCqh
    goBp4TmI
    =ly6S
    -----END PGP SIGNATURE-----
    

    Traceback (most recent call last): File "<string>", line 1, in <module> FileNotFoundError: [Errno 2] No such file or directory: '/tmp/ghs_sig_71f8dc5f312c2dc020bbeb544189f15f -.ots'

    </details>

  182. DrahtBot added the label Needs rebase on Jun 10, 2021
  183. ajtowns force-pushed on Jun 11, 2021
  184. ajtowns commented at 2:27 AM on June 11, 2021: contributor

    Rebased past #22141 ; renamed versionbitscache to g_versionbitscache ; addressed @MarcoFalke's comments

  185. DrahtBot removed the label Needs rebase on Jun 11, 2021
  186. MarcoFalke approved
  187. MarcoFalke commented at 6:36 AM on June 11, 2021: member

    re-ACK f41c075cd2b1f0a38e645b58615d5f8a1bb8af71 😲

    only changes:

    • remove inline and struct keywords where not needed
    • rebase
    • Add g_ prefix to versionbitscache

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-ACK f41c075cd2b1f0a38e645b58615d5f8a1bb8af71 😲
    
    only changes:
    * remove inline and struct keywords where not needed
    * rebase
    * Add g_ prefix to versionbitscache
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUhv0gwAgxHsyNWf9+Zb59KLD/mAw/JOdA9SvkU/f/QI4qXoslOAaD5MdE546paw
    qM5g00MVUaLsgwoLqIgI5pfAiUpT/wSRChM2fIWzK0QqZPaq/9/y6az8Q4vT1ukM
    a0+zp86egBrWn/bB2rJuF5KAP7yzqQ3V+rWJ5NOit7DEuZ7bZbBerWxUO+zy9W8j
    rKAR3peiyl0Yk06J4fftH4OnUBnN4t9o/GZQjWrIUxTMtiSLvOprmjJsdWeObwP8
    Qxd7ANtZypHSwrqBqnl3+sTJCrx7pt1CI/8y4WvbXRTVlJihg46mh7IrY0wDv2D/
    susYpZy6kXTLaDG2G6IZ2u79iC/t8GCCCQjWwEM8z05/ay3MBAyAJtVUM+FaDtxs
    ye1E2jOOgdfieeWrxqATCxpl+ChsQPcARm8ksrhIrfJECV7rczvy0wXpTUfCM+F3
    L6jEdNwh0D0nFBNd0mz7/djxbHdSsEZqSlDaJp3goz8F/KflcriGQ9U0Y7Zt1M2I
    rZZfXr4D
    =oC8X
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash ef1b7f61a9b0e9fe191ad81738ce361724107d785fda2888ecfb8e54fd721e5b -

    </details>

  188. jnewbery commented at 4:10 PM on June 11, 2021: member

    reACK f41c075cd2b1f0a38e645b58615d5f8a1bb8af71

  189. DrahtBot added the label Needs rebase on Jun 12, 2021
  190. ajtowns force-pushed on Jun 12, 2021
  191. ajtowns commented at 9:09 AM on June 12, 2021: contributor

    Rebased past #21866

  192. DrahtBot removed the label Needs rebase on Jun 12, 2021
  193. MarcoFalke commented at 4:01 PM on June 13, 2021: member

    re-ACK 01c156677e34e21591e71fb979a041f19c060018 🔵

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-ACK 01c156677e34e21591e71fb979a041f19c060018 🔵
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUimtAwAxwW2oHhca0vBquCwKsoXJ1HPQCWPUS0yXCXiKL8J/5ojI8gXEw7Pv1Ug
    yzYIlNsr30ixEWXZCOZ4O+aRW8E9WvDy4JKesZ/xrBt37I66JLnOu95futuAcIfG
    6+L4rMRTqPnX+6wHx5WzVfBFhwN7eVjQbhu/Mia7FJsXHjeiPs6mjKYXUQzx7Ipi
    xYy2UxRfLBk98vABaw3969QTytAKxxwln8w3plPV6HVqonroUjGKQqlWsmFkPn9C
    yXFJwwWSsJQhJZgbyayhujK2HPaGIIO4pN2lrTdk5CnHS8vaA6b06OAvLhTNKtSA
    l+IRRHwtRToIp8/bngTkTB4vvccB2o50/ty+aVk7mlP4FmMCHxfXAa7o7g6WykQW
    efobheQdOAhpC1aV5EtlvCnQYlD3BMB806fxRQCzCa1FDJnG6jmH7+zkmKUbD9dx
    0+b6xQIY1ROnz9cHqT1K+0w/9S2X4O4GBJn27TE4lCfuTwfoE/tkQI1xbGgaNN/t
    YKXw60G8
    =L+EA
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash 3467c5ea999c8549d102168d19450d004f51c5fbdba853fb143538b01ab1893c -

    </details>

  194. in src/deploymentstatus.h:52 in e3a93b5095 outdated
      47 |  }
      48 |  
      49 | +inline bool DeploymentEnabled(const Consensus::Params& params, Consensus::DeploymentPos dep)
      50 | +{
      51 | +    assert(Consensus::ValidDeployment(dep));
      52 | +    return params.vDeployments[dep].nTimeout != 0;
    


    MarcoFalke commented at 6:53 AM on June 14, 2021:

    e3a93b5095522f9315942600cbeb394a902bc5ce: This is currently unused, so there are no bugs. However, I am wondering if the implementation is the most meaningful. This is the first place where a magic value of 0 for the timeout is defined. Be aware that nTimeout is mostly an unsanitized [1] "don't care value", as the enabled status is read from nStartTime (NEVER_ACTIVE).

    Finally, I am wondering what the use of this helper might be. Maybe delete the function until there is a use case?

    template <class> inline constexpr bool ALWAYS_FALSE{false};
    template <class Dep> bool DeploymentEnabled(const Consensus::Params& params, Dep dep)
    {
    static_assert(ALWAYS_FALSE<Dep>, "DeploymentEnabled only meaningful for buried deployments");
    

    [1] There was a favour to not sanitize this value across all ST implementations. #21392 (review)

    (Obviously non-blocking nit, can be fixed up later)


    ajtowns commented at 4:30 AM on June 15, 2021:

    This is the first place where a magic value of 0 for the timeout is defined.

    != 0 matches the behaviour used for IsScriptWitnessEnabled prior to 0328dcdcfcb56dc8918697716d7686be048ad0b3

    Finally, I am wondering what the use of this helper might be. Maybe delete the function until there is a use case?

    Having it defined makes it (relatively) easy to switch DEPLOYMENT_SEGWIT back to a signaled deployment (ie updating consensus/params.h and chainparams.cpp) and checking that the change does what it's supposed to (ie not require updating validation.cpp etc).

    the enabled status is read from nStartTime (NEVER_ACTIVE).

    I think it would make sense to switch it to this -- should be possible to have a "fake" NEVER_ACTIVE that still passes DeploymentEnabled by setting the starttime to greater than uint32_t::max(). (Might do it if there's another rebase needed, otherwise will leave for a later PR)


    MarcoFalke commented at 6:09 PM on July 1, 2021:
  195. jnewbery commented at 8:29 AM on June 14, 2021: member

    ACK 01c156677e

  196. DrahtBot cross-referenced this on Jun 14, 2021 from issue Move CBlockTreeDB to node/blockstorage by MarcoFalke
  197. ajtowns added this to the milestone 22.0 on Jun 21, 2021
  198. michaelfolkson commented at 11:38 AM on June 21, 2021: contributor

    Concept ACK, Approach ACK.

    Looked at this at the PR review club session back in March 2021 and changes since then appear to be minimal. This PR doesn't appear to be urgent for 22.0 as Taproot deployment won't be buried anytime soon but this PR has been open for a while so it would definitely be nice to get it in.

  199. ajtowns commented at 6:49 AM on June 23, 2021: contributor

    Including this in 22.0 makes it easier to switch taproot to being a buried deployment, which would allow removing the speedy trial code, which in turn may make it easier to backport new activation code for any new soft forks that may be ready for deployment prior to version 22 reaching end-of-life (perhaps some of the consensus cleanup ideas eg).

  200. MarcoFalke commented at 7:07 AM on June 23, 2021: member

    backport new activation code for any new soft forks that may be ready for deployment prior to version 22 reaching end-of-life

    I think generally we only backport soft-forks to the previous major version. Though I have a slight preference to merge this for 22 to minimize potential conflicts when backporting (other) consensus changes.

  201. in src/deploymentinfo.cpp:35 in 80b5b91752 outdated
      30 | +    case Consensus::DEPLOYMENT_CSV:
      31 | +        return "csv";
      32 | +    case Consensus::DEPLOYMENT_SEGWIT:
      33 | +        return "segwit";
      34 | +    } // no default case, so the compiler can warn about missing cases
      35 | +    return "";
    


    fjahr commented at 3:40 PM on June 26, 2021:

    nit: Could have given a default that provides more context like "unknown"


    MarcoFalke commented at 5:23 PM on June 26, 2021:

    Shouldn't matter too much, as it is dead code. Empty string might even make it easier to run CHECK_NONFATAL(!name.empty()); in rpc code.


    MarcoFalke commented at 7:29 AM on June 30, 2021:

    Or just assert(false) for symmetry with the assert at the beginning of the function?

  202. in src/versionbits.cpp:220 in 0802af87b7 outdated
     216 | +
     217 | +int32_t ComputeBlockVersion(const CBlockIndex* pindexPrev, const Consensus::Params& params)
     218 | +{
     219 | +    int32_t nVersion = VERSIONBITS_TOP_BITS;
     220 | +
     221 | +    for (int i = 0; i < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; i++) {
    


    fjahr commented at 3:57 PM on June 26, 2021:

    nit: could have upgraded to static_cast<int>(Consensus::MAX_VERSION_BITS_DEPLOYMENTS)


    MarcoFalke commented at 5:25 PM on June 26, 2021:

    u{Consensus::MAX_VERSION_BITS_DEPLOYMENTS} is even more preferable when this is changed. (with u being the underlying type)


    MarcoFalke commented at 7:44 AM on June 28, 2021:

    See also #19438 (review)

  203. fjahr commented at 5:02 PM on June 26, 2021: contributor

    re-utACK 01c156677e34e21591e71fb979a041f19c060018

  204. MarcoFalke commented at 9:08 AM on June 27, 2021: member

    Needs rebase.

    Feedback can be ignored or done in a follow-up if needed, to simplify re-review.

  205. ajtowns force-pushed on Jun 28, 2021
  206. ajtowns commented at 3:19 AM on June 28, 2021: contributor

    Thanks @MarcoFalke ; rebased past #22156 ; leaving nit fixing 'til post 22.0

  207. in src/validation.cpp:687 in 24972d9f49 outdated
     683 | @@ -684,9 +684,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     684 |      }
     685 |  
     686 |      // Check for non-standard pay-to-script-hash in inputs
     687 | -    const auto& params = args.m_chainparams.GetConsensus();
     688 | -    auto taproot_state = VersionBitsState(m_active_chainstate.m_chain.Tip(), params, Consensus::DEPLOYMENT_TAPROOT, versionbitscache);
     689 | -    if (fRequireStandard && !AreInputsStandard(tx, m_view, taproot_state == ThresholdState::ACTIVE)) {
     690 | +    const bool taproot_active = DeploymentActiveAt(*m_active_chainstate.m_chain.Tip(), args.m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);
    


    MarcoFalke commented at 7:42 AM on June 28, 2021:

    24972d9f49:

    Shouldn't this say:

    const bool taproot_active{DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), args.m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_TAPROOT)};
    

    ajtowns commented at 1:25 AM on June 30, 2021:

    Yes, it should, fixed.

  208. MarcoFalke changes_requested
  209. MarcoFalke commented at 7:43 AM on June 28, 2021: member

    re-review e14966ee31f02e359896dbb72e2627b3ae12a314 🏙

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-review e14966ee31f02e359896dbb72e2627b3ae12a314 🏙
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUgN8Qv9GgVO/0B/5pXOElsDfA01j0nQa49UyijPA+VnSucn3IODX70Qpreqsutf
    LtBbFvY5YG7elclunOovo53MDHmtXHQasTqEyubDzlOn4sGAuUSHgbiqQEN1KUTE
    gkdto79U3h7Fi8uUwwkEKpsKtaCPnUXHoqfa5OomM3K4Ku2r9FOEmCshP/JRNcy6
    WlDKFZd5N+UAf+4wqwDr9Oo0WCde/vadC3vU6CB4EDNID2WIX6PIK412wF+3/Z/4
    KjXkIBgIrggK/OHS2cXaGdRJuskc9EPZa+n6+PC1UxQHS3nWsTmNQRf9b97zDovh
    PygV0L/fv8BVn3lH0DPh94IhsMTOJge07WPcPj2E3beGEDjFZJC12GgjZa+EMNBi
    yKuwc2QucVhEfrUyjmIKgqtylpjYVNApdvgNHFTZGwDE8w/m9MndjkUPZDuUm3E7
    C2IO9DGmjC8T4FsWuuogmvyF5/0QJ99924B7BWRiFM9hsbUEwG1akHgSQ8ez1YJK
    yweuMjpk
    =i3Iu
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash de786aee714e95ba66b851bc0e29b843b5f1124ce723a9bc15d6d531f0fb1477 -

    </details>

  210. DrahtBot added the label Needs rebase on Jun 29, 2021
  211. versionbits: correct doxygen comments 36a4ba0aaa
  212. versionbits: Use dedicated lock instead of cs_main eccd736f3d
  213. [refactor] Add deploymentstatus.h
    Provides DeploymentEnabled, DeploymentActiveAt, and DeploymentActiveAfter
    helpers for checking the status of buried deployments. Can be overloaded
    so the same syntax works for non-buried deployments, allowing future
    soft forks to be changed from signalled to buried deployments without
    having to touch the implementation code.
    
    Replaces IsWitnessEnabled and IsScriptWitnessEnabled.
    2b0d291da8
  214. ajtowns force-pushed on Jun 29, 2021
  215. ajtowns commented at 10:04 PM on June 29, 2021: contributor

    Rebased past #21789

  216. [refactor] Add versionbits deployments to deploymentstatus.h
    Adds support for versionbits deployments to DeploymentEnabled,
    DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache
    from validation to deploymentstatus.
    de55304f6e
  217. scripted-diff: rename versionbitscache
    -BEGIN VERIFY SCRIPT-
    sed -i -e 's/versionbitscache/g_versionbitscache/g' $(git grep -l versionbitscache)
    -END VERIFY SCRIPT-
    c64b2c6a0f
  218. [move-only] Rename versionbitsinfo to deploymentinfo ea68b3a572
  219. deploymentinfo: Add DeploymentName() 92f48f360d
  220. [refactor] rpc/blockchain.cpp: SoftForkPushBack
    Rename BIP9SoftForkPushBack and BuriedSoftForkPushBack to SoftForkPushBack
    and have the compiler figure out which one to use based on the deployment
    type. Avoids the need to update the file when burying a deployment.
    8ee3e0bed5
  221. [refactor] versionbits: make VersionBitsCache a full class
    Moves the VersionBits* functions to be methods of the cache class,
    and makes the cache and its lock private to the class.
    0cfd6c6a8f
  222. [move-only] Move ComputeBlockVersion from validation to versionbits 4a69b4dbe0
  223. [refactor] Move ComputeBlockVersion into VersionBitsCache
    This also changes ComputeBlockVersion to take the versionbits cache
    mutex once, rather than once for each versionbits deployment.
    c5f36725e8
  224. tests: remove ComputeBlockVersion shortcut from versionbits tests e48826ad87
  225. DrahtBot removed the label Needs rebase on Jun 29, 2021
  226. ajtowns force-pushed on Jun 30, 2021
  227. ajtowns requested review from MarcoFalke on Jun 30, 2021
  228. MarcoFalke approved
  229. MarcoFalke commented at 7:24 AM on June 30, 2021: member

    re-ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3 🥈

    Checked with: git range-diff bitcoin-core/master e14966ee31f02e359896dbb72e2627b3ae12a314 e48826ad87b4f92261f7433e84f48dac9bd9e5c3 --word-diff-regex=. -U0

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3 🥈
    
    Checked with:
    git range-diff bitcoin-core/master e14966ee31f02e359896dbb72e2627b3ae12a314 e48826ad87b4f92261f7433e84f48dac9bd9e5c3 --word-diff-regex=. -U0
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUh+dAv+NVpZKf22BScmqPb4YLJH2snw87TwSnr6Ez2ocyL441p57JrElilouzMN
    gklFgwx0RYY2WlGKtGVub1aa3gVLqmDbdWKi6sGxTJB+5tbQnaK4S5tEDKAWfPDu
    q7xawsZ7mOKeyIJXlRrwMiipeziVGtuDW7pQ5OkfCfQevvCMmexdA/vwigKR9UbW
    adVkvMNwkA1g8LLbfn3xMUGk3QnidnYyaJtP+55LK91kLMx/5fDgmEoj5kBFYu1s
    8n5SE4UW06RfkUfAuPJT5TgM3IjxoHmO9bwQbdh27a/1yY+zTggjy9Ak8cWs3xAi
    jr3NjAohqfp1EKKMP8/oGTUn7AHMUu/tUPzqWH0iTJ2NdGt8HHrT54qBSB7ryS/W
    +rfMOv0G3quOWCd0jc5zQLHsaMndlItfPgRMWBgMQ255zo0hWBOs6HV9Xe1kuZgF
    4jCLR/KvxoK9/e0ore9XqEDpyeABo0oRwzVSg0XrHIMWHSpB2qOZx1ycQwqnoaR4
    Y722yBJ3
    =PNtg
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash 51b700b2e20a4085313cce0dc833915df20496b87f40dbee5ed2e7651008877d -

    </details>

  230. DrahtBot cross-referenced this on Jun 30, 2021 from issue Move pblocktree global to BlockManager by MarcoFalke
  231. jnewbery commented at 3:50 PM on June 30, 2021: member

    ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3

  232. MarcoFalke merged this on Jul 1, 2021
  233. MarcoFalke closed this on Jul 1, 2021

  234. bitcoin deleted a comment on Jul 1, 2021
  235. bitcoin deleted a comment on Jul 1, 2021
  236. bitcoin deleted a comment on Jul 1, 2021
  237. bitcoin deleted a comment on Jul 1, 2021
  238. bitcoin deleted a comment on Jul 1, 2021
  239. bitcoin deleted a comment on Jul 1, 2021
  240. sidhujag referenced this in commit 560ae219d8 on Jul 3, 2021
  241. laanwj removed this from the "Blockers" column in a project

  242. ajtowns cross-referenced this on Jul 13, 2021 from issue refactor: Use DeploymentEnabled to hide VB deployments by MarcoFalke
  243. tryphe cross-referenced this on Jul 30, 2021 from issue gcc -Wtype-limits warnings in consensus/params.h by Rspigler
  244. gwillen referenced this in commit 47164a1e9c on Jun 1, 2022
  245. bitcoin locked this on Aug 16, 2022

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: 2026-05-20 06:54 UTC