fix uninitialized variable nMinerConfirmationWindow #17449

pull ghost wants to merge 1 commits into bitcoin:master from changing 1 files +2 −2
  1. ghost commented at 3:50 am on November 12, 2019: none
    It is used for the computation of BIP9WarningHeight, and by that time it isn’t initialized.
  2. DrahtBot added the label Validation on Nov 12, 2019
  3. ghost commented at 5:47 am on November 12, 2019: none
    I find it scary, that a bug like this can be introduced. I am wondering if there are not any tools that check the code for references to uninitialized variables?
  4. DrahtBot commented at 6:54 am on November 12, 2019: member

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

    Conflicts

    No conflicts as of last run.

  5. promag commented at 8:33 am on November 12, 2019: member
    Fixes what? Both are already initialized..
  6. laanwj commented at 9:07 am on November 12, 2019: member

    Fixes what? Both are already initialized..

    It’s used for the computation of BIP9WarningHeight, and by that time it isn’t initialized (I missed it first, too!).

    I find it scary, that a bug like this can be introduced. I am wondering if there are not any tools that check the code for references to uninitialized variables?

    The tests are being run in various sanitizers, and valgrind can do this IIRC. I’m surprised it wasn’t caught.

    ACK 6fcd798b42569130f85328133e2124476fb2a42a

    Should fix #16697.

  7. laanwj added the label Needs backport (0.19) on Nov 12, 2019
  8. practicalswift commented at 9:09 am on November 12, 2019: contributor

    @promag It looks like consensus.nMinerConfirmationWindow was indeed being read uninitialised on L74 prior to the change suggested by this PR?

    0consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
    

    This code was introduced in fdb3e8f8b27e3b0b2f88c32915975c6e4c299b1e in PR #16713 which was merged in to master on September 27, 2019.

    Really fascinating that this 1.) wasn’t caught automatically by our static and dynamic analysis, 2.) wasn’t caught by our review process and 3.) was unnoticed in master for over a month. Room for improvement! :)

  9. practicalswift commented at 9:13 am on November 12, 2019: contributor

    @bitcoinVBR

    Thanks a lot for reporting this! What an excellent first time contribution! Hope to see more great contributions from you.

    Sorry about the initial misunderstanding in your original PR #17433 :)

  10. laanwj commented at 9:15 am on November 12, 2019: member

    Really fascinating that this 1.) wasn’t caught automatically by our static and dynamic analysis,

    Port to rust when? :smile:

  11. promag commented at 9:15 am on November 12, 2019: member
    Oh!
  12. practicalswift commented at 10:55 am on November 12, 2019: contributor

    Worth noting is that the read of nMinerConfirmationWindow takes place on mainnet and testnet but not on regtest:

     0$ grep -E '(nMinerConfirmationWindow|CBaseChainParams|MinBIP9WarningHeight)' \
     1      src/chainparams.cpp | grep -A2 " = *CBaseChainParams"
     2        strNetworkID = CBaseChainParams::MAIN;
     3        consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
     4        consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
     5        strNetworkID = CBaseChainParams::TESTNET;
     6        consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
     7        consensus.nMinerConfirmationWindow = 2016; // nPowTargetTimespan / nPowTargetSpacing
     8        strNetworkID =  CBaseChainParams::REGTEST;
     9        consensus.MinBIP9WarningHeight = 0;
    10        consensus.nMinerConfirmationWindow = 144; // Faster than normal for regtest (144 instead of 2016)
    
  13. theStack commented at 12:05 pm on November 12, 2019: member
    ACK https://github.com/bitcoin/bitcoin/pull/17449/commits/6fcd798b42569130f85328133e2124476fb2a42a Really surprising that apparently no compiler spit out a warning about this uninitialized access…
  14. laanwj commented at 12:16 pm on November 12, 2019: member

    Worth noting is that the read of nMinerConfirmationWindow takes place on mainnet and testnet but not on regtest:

    Good catch. If there is no unit test that instantiates the main / testnet chain, that’d explain why sanitizer CI checks don’t notice it. But it seems that quite a few do, for example:

    0pow_tests.cpp:    const auto consensus = CreateChainParams(CBaseChainParams::MAIN)->GetConsensus();
    
  15. practicalswift commented at 12:37 pm on November 12, 2019: contributor

    @laanwj I think there might be a more subtle issue here too - note that MinBIPWarningHeight appears to have the correct value (481824 + 2016 == 483840) when reaching Condition(…) on mainnet: #16713 (comment).

    I think we might be chasing two issues here where the uninitialised read is the first one :)

  16. MarcoFalke commented at 12:48 pm on November 12, 2019: member

    Good catch. If there is no unit test that instantiates the main / testnet chain, that’d explain why sanitizer CI checks don’t notice it. But it seems that quite a few do, for example:

    We have functional tests that run on the testnet chain. So this should have been caught by a sanitizer (assuming one was enabled)

  17. MarcoFalke commented at 1:19 pm on November 12, 2019: member
    I don’t think we have the memory sanitizer enabled anywhere
  18. ghost commented at 1:39 pm on November 12, 2019: none

    @bitcoinVBR

    Thanks a lot for reporting this! What an excellent first time contribution! Hope to see more great contributions from you.

    Sorry about the initial misunderstanding in your original PR #17433 :)

    Thanks! I’m happy to help. I actually only found this bug because of my work on BitcoinV. I’ll continue to keep an eye out for anything fishy.

  19. practicalswift commented at 1:51 pm on November 12, 2019: contributor

    Compiler writers get a lot of heat for exploiting UB to the fullest for optimisation purposes, but in this case it seems like we got really lucky – they “fixed” our code (assuming Clang -O2) 😉

    Look at the Clang -O2 case where we get the 483840 (481824 + 2016 == 483840) we intended (!) :)

     0$ clang++-8 -O0 -o repro repro.cpp && ./repro
     1consensus.MinBIP9WarningHeight[1] == 481824
     2consensus.MinBIP9WarningHeight[2] == 4678000
     3consensus.MinBIP9WarningHeight[3] == 481824
     4$ clang++-8 -O1 -o repro repro.cpp && ./repro
     5consensus.MinBIP9WarningHeight[1] == 586474128
     6consensus.MinBIP9WarningHeight[2] == 4678896
     7consensus.MinBIP9WarningHeight[3] == -1100057664
     8$ clang++-8 -O2 -o repro repro.cpp && ./repro
     9consensus.MinBIP9WarningHeight[1] == 483840
    10consensus.MinBIP9WarningHeight[2] == 483840
    11consensus.MinBIP9WarningHeight[3] == 483840
    12$ g++-8 -O0 -o repro repro.cpp && ./repro
    13consensus.MinBIP9WarningHeight[1] == 503884
    14consensus.MinBIP9WarningHeight[2] == 481824
    15consensus.MinBIP9WarningHeight[3] == 503884
    16$ g++-8 -O1 -o repro repro.cpp && ./repro
    17consensus.MinBIP9WarningHeight[1] == 481824
    18consensus.MinBIP9WarningHeight[2] == 481824
    19consensus.MinBIP9WarningHeight[3] == 481824
    20$ g++-8 -O2 -o repro repro.cpp && ./repro
    21consensus.MinBIP9WarningHeight[1] == 481824
    22consensus.MinBIP9WarningHeight[2] == 481824
    23consensus.MinBIP9WarningHeight[3] == 481824
    24$ cat repro.cpp
    25#include <cstdint>
    26#include <iostream>
    27
    28namespace Consensus {
    29struct Params {
    30  int SegwitHeight;
    31  int MinBIP9WarningHeight;
    32  uint32_t nMinerConfirmationWindow;
    33};
    34} // namespace Consensus
    35
    36class CChainParams {
    37public:
    38  const Consensus::Params &GetConsensus() const { return consensus; }
    39
    40protected:
    41  CChainParams() {}
    42
    43  Consensus::Params consensus;
    44};
    45
    46class CMainParams : public CChainParams {
    47public:
    48  CMainParams() {
    49    consensus.SegwitHeight = 481824;
    50    consensus.MinBIP9WarningHeight =
    51        consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
    52    consensus.nMinerConfirmationWindow = 2016;
    53  }
    54};
    55
    56int main() {
    57  CMainParams main_params_1;
    58  std::cout << "consensus.MinBIP9WarningHeight[1] == "
    59            << main_params_1.GetConsensus().MinBIP9WarningHeight << std::endl;
    60
    61  CMainParams main_params_2;
    62  std::cout << "consensus.MinBIP9WarningHeight[2] == "
    63            << main_params_2.GetConsensus().MinBIP9WarningHeight << std::endl;
    64
    65  CMainParams main_params_3;
    66  std::cout << "consensus.MinBIP9WarningHeight[3] == "
    67            << main_params_3.GetConsensus().MinBIP9WarningHeight << std::endl;
    68}
    

    :)

  20. practicalswift commented at 1:56 pm on November 12, 2019: contributor

    @jonatack The output you posted from the testing of PR #16713 in #16713 (comment)

    02019-09-20T11:26:36Z SegwitHeight 481824
    12019-09-20T11:26:36Z nMinerConfirmationWindow 2016
    22019-09-20T11:26:36Z MinBIPWarningHeight 483840
    32019-09-20T11:26:36Z pindex->nHeight 164836
    42019-09-20T11:26:36Z pindex->nVersion 1
    52019-09-20T11:26:36Z ComputeBlockVersion 536870912
    

    … could it be the case that you happened to test with Clang with the project default optimisation level -O2? :) If so, could you retry using GCC (any optimisation level), or Clang with -O0 or -O1 and see if you get the same results?

  21. MarcoFalke commented at 2:16 pm on November 12, 2019: member
    @practicalswift I tried gcc 9.2.1 and it happened to optimize out the bug for me as well. Though running the same binary in valgrind still jells at me, so I can’t make any sense out of that.
  22. practicalswift commented at 2:24 pm on November 12, 2019: contributor
    @MarcoFalke What is the output of ./repro for binaries compiled at the different optimisation levels?
  23. jonatack commented at 2:41 pm on November 12, 2019: member

    could it be the case that you happened to test with Clang with the project default optimisation level -O2? :) If so, could you retry using GCC (any optimisation level), or Clang with -O0 or -O1 and see if you get the same results?

    It was with gcc version 8.3.0 (Debian 8.3.0-6) with no optimisation flags.

  24. MarcoFalke commented at 2:49 pm on November 12, 2019: member

    It was with gcc version 8.3.0 (Debian 8.3.0-6) with no optimisation flags.

    The default is O2 for Bitcoin Core

  25. jnewbery commented at 3:07 pm on November 12, 2019: member

    I’d prefer to just hard code the MinBIP9WarningHeight, eg:

    0        consensus.MinBIP9WarningHeight = 483840; // segwit activation height + miner confirmation window
    

    Hard-coding the value means that even if the member initializations are moved around again, nMinerConfirmationWindow will not be accessed before initialization.

    MinBIP9WarningHeight is currently the only member variable that is set from the value of a different member, except the hashGenesisBlock which is checked with an assert against a hard-coded value.

  26. MarcoFalke commented at 3:24 pm on November 12, 2019: member
    I was about to suggest that the members should be marked const, so that initialization and initialization order is enforced by the compiler. Though, I believe that makes the code less readable as C++ does not have named parameters that can be passed into a constructor.
  27. MarcoFalke commented at 4:12 pm on November 12, 2019: member
    @bitcoinVBR Could you please adjust the patch as suggested by @jnewbery and then squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
  28. fanquake added the label Waiting for author on Nov 12, 2019
  29. JayMedyaa approved
  30. JayMedyaa commented at 10:44 pm on November 12, 2019: none
    Has been fixed.
  31. fix uninitialized variable nMinerConfirmationWindow
    fix uninitialized variable hard code the MinBIP9WarningHeight
    
    fix uninitialized var hard code the MinBIP9WarningHeight instead
    edb6b768a4
  32. jnewbery commented at 0:22 am on November 13, 2019: member
    utACK edb6b768a
  33. promag commented at 0:29 am on November 13, 2019: member
    ACK edb6b768a4185a4aaa6281ee50a6538f7426cb1e, commit description could be cleaned up though.
  34. MarcoFalke removed the label Waiting for author on Nov 13, 2019
  35. MarcoFalke commented at 0:56 am on November 13, 2019: member

    ACK edb6b768a4185a4aaa6281ee50a6538f7426cb1e, used python3 to do the addition locally 📍

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK edb6b768a4185a4aaa6281ee50a6538f7426cb1e, used python3 to do the addition locally 📍
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUgIZAwAmxU36N8KM439sYBxtHQ+TI9Qy+RaHDVhm6ccEcQfGXIh0O/AhPyqwRmE
     8kq/t2Ukk2tp6tya/qGNTYqGXBvgMIcH7w7QcWSt+/kcGz/t5isrMG3I9A6IB4ZVn
     9DGC57cd3A+kge+DZkp5pQcTqHZvSKVFD0jCF0yF5ui6+2u7yyCCY3YaWJfaqvZt1
    10w6ZYv3VBLPBgls3PLzzU+iho/GKivNlGSaReFdynlZ57gzrWt9E4eQMIifkf5tNo
    11DBiJh5Jeodf8C/j6IDiL6l9gzPXUFzxLCoOctJu9uvHpnT5GXNO5GfeUmNDjLJYp
    1272gYa7/UZkg+kl1a0mF32UNhzm4PObfeQn9UqH0MwTuJzdOgGqaNIwz6oISPdNd2
    131pwXaBsbfgrusHKPd8GXlFFhfm7P+ICNwDUJA5aJWvxPkbQn4ZM2l+OXqUryP9xW
    1476tR8c9G0xP7sYDNwfEKp5x9BaSSeR/f9dauOwndRMYazqLYRrb3p8vKDa8yHcs5
    15pjdRxFk8
    16=te4h
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 346fefa7ddee2dc8d6ced4fd005240f5e06d18da505f8b0edfea1d7208fcec73 -

  36. practicalswift commented at 9:32 am on November 14, 2019: contributor
    ACK edb6b768a4185a4aaa6281ee50a6538f7426cb1e, used clang++ -O2 on the previous version^W^W^W^W^W^Wbc to verify the addition locally 🏓
  37. Sjors commented at 12:03 pm on November 14, 2019: member
    Code review ACK edb6b76. Nit: commit description has duplicate text.
  38. practicalswift commented at 1:10 pm on November 14, 2019: contributor

    FWIW, Valgrind warns about this when running a bitcoind binary compiled from current master using GCC with -O0:

     0==19043== Thread 25 b-msghand:
     1==19043== Conditional jump or move depends on uninitialised value(s)
     2==19043==    at 0x49E12D: WarningBitsConditionChecker::Condition(CBlockIndex const*, Consensus::Params const&) const (validation.cpp:1815)
     3==19043==    by 0x541AED: AbstractThresholdConditionChecker::GetStateFor(CBlockIndex const*, Consensus::Params const&, std::map<CBlockIndex const*, ThresholdState, std::less<CBlockIndex const*>, std::allocator<std::pair<CBlockIndex const* const, ThresholdState> > >&) const (versionbits.cpp:70)
     4==19043==    by 0x484248: UpdateTip(CBlockIndex const*, CChainParams const&) (validation.cpp:2379)
     5==19043==    by 0x4854A1: CChainState::ConnectTip(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) (validation.cpp:2584)
     6==19043==    by 0x485FD8: CChainState::ActivateBestChainStep(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) (validation.cpp:2715)
     7==19043==    by 0x4866B2: CChainState::ActivateBestChain(BlockValidationState&, CChainParams const&, std::shared_ptr<CBlock const>) (validation.cpp:2836)
     8==19043==    by 0x48CFAC: ProcessNewBlock(CChainParams const&, std::shared_ptr<CBlock const>, bool, bool*) (validation.cpp:3792)
     9==19043==    by 0x21AA69: ProcessMessage(CNode*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CDataStream&, long, CChainParams const&, CConnman*, BanMan*, std::atomic<bool> const&) (net_processing.cpp:2974)
    10==19043==    by 0x21D80F: PeerLogicValidation::ProcessMessages(CNode*, std::atomic<bool>&) (net_processing.cpp:3331)
    11==19043==    by 0x1BDD83: CConnman::ThreadMessageHandler() (net.cpp:2013)
    12==19043==    by 0x202E2F: void std::__invoke_impl<void, void (CConnman::*&)(), CConnman*&>(std::__invoke_memfun_deref, void (CConnman::*&)(), CConnman*&) (invoke.h:73)
    13==19043==    by 0x1FE55C: std::__invoke_result<void (CConnman::*&)(), CConnman*&>::type std::__invoke<void (CConnman::*&)(), CConnman*&>(void (CConnman::*&)(), CConnman*&) (invoke.h:95)
    

    @bitcoinVBR Thanks again for reporting! May I ask how you noticed this issue? :)

  39. promag commented at 1:45 pm on November 14, 2019: member
    @practicalswift he said how above #17449 (comment)
  40. practicalswift commented at 2:43 pm on November 14, 2019: contributor
    @promag Yes, I understood that it was found in the context of @bitcoinVBR’s work on a fork, but I didn’t get how it was found. More specifically if it was caught by manual source code review, static analysis or dynamic analysis :)
  41. ghost commented at 3:29 pm on November 14, 2019: none

    I was updating BitcoinV to add the variable block reward feature based on Bitcoin v0.19.0 and my eye noticed the variable used before being initialized.

    Are there any bounty rewards for these finds?

    17yVfEbGZqX1Ce4qTmv4QD29DfeohPq2zi

    Thanks, BitcoinV

    Sent from my iPhone

    On Nov 14, 2019, at 7:13 AM, practicalswift notifications@github.com wrote:

     FWIW, Valgrind warns about this when running a bitcoind binary compiled from current master using GCC with -O0:

    ==19043== Thread 25 b-msghand: ==19043== Conditional jump or move depends on uninitialised value(s) ==19043== at 0x49E12D: WarningBitsConditionChecker::Condition(CBlockIndex const*, Consensus::Params const&) const (validation.cpp:1815) ==19043== by 0x541AED: AbstractThresholdConditionChecker::GetStateFor(CBlockIndex const*, Consensus::Params const&, std::map<CBlockIndex const*, ThresholdState, std::less<CBlockIndex const*>, std::allocator<std::pair<CBlockIndex const* const, ThresholdState> > >&) const (versionbits.cpp:70) ==19043== by 0x484248: UpdateTip(CBlockIndex const*, CChainParams const&) (validation.cpp:2379) ==19043== by 0x4854A1: CChainState::ConnectTip(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr const&, ConnectTrace&, DisconnectedBlockTransactions&) (validation.cpp:2584) ==19043== by 0x485FD8: CChainState::ActivateBestChainStep(BlockValidationState&, CChainParams const&, CBlockIndex*, std::shared_ptr const&, bool&, ConnectTrace&) (validation.cpp:2715) ==19043== by 0x4866B2: CChainState::ActivateBestChain(BlockValidationState&, CChainParams const&, std::shared_ptr) (validation.cpp:2836) ==19043== by 0x48CFAC: ProcessNewBlock(CChainParams const&, std::shared_ptr, bool, bool*) (validation.cpp:3792) ==19043== by 0x21AA69: ProcessMessage(CNode*, std::__cxx11::basic_string<char, std::char_traits, std::allocator > const&, CDataStream&, long, CChainParams const&, CConnman*, BanMan*, std::atomic const&) (net_processing.cpp:2974) ==19043== by 0x21D80F: PeerLogicValidation::ProcessMessages(CNode*, std::atomic&) (net_processing.cpp:3331) ==19043== by 0x1BDD83: CConnman::ThreadMessageHandler() (net.cpp:2013) ==19043== by 0x202E2F: void std::__invoke_impl<void, void (CConnman::&)(), CConnman&>(std::__invoke_memfun_deref, void (CConnman::&)(), CConnman&) (invoke.h:73) ==19043== by 0x1FE55C: std::__invoke_result<void (CConnman::&)(), CConnman&>::type std::__invoke<void (CConnman::&)(), CConnman&>(void (CConnman::&)(), CConnman&) (invoke.h:95) @bitcoinVBR Thanks again for reporting! May I ask how you noticed this issue? :)

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

  42. practicalswift commented at 4:21 pm on November 14, 2019: contributor

    Are there any bounty rewards for these finds?

    The Bitcoin Core project does not have any official bounty program AFAIK, but the is nothing stopping individuals from donating to other individuals who they think have done valuable security research.

    (Aside: My personal view is that we could do a much better job recognising good security research: especially the non-glamorous kind of low-level security research. As an example: if we had a proper incentive structure in place I guess someone would be running bitcoind binaries compiled from master with -O{0,1,2} running under valgrind around the clock just waiting to report an issue like this. Incentives matter. Unfortunately I don’t know the optimal incentive structure (the mix between public recognition vs official bug bounty vs donations vs just being polite and thanking researchers vs long-term funding, etc.), but I’m fairly certain we’re not at the global optimum right now :))

    Shameless plug: People interested in running bitcoind under valgrind might want to help out by reviewing #17455 :)

  43. fanquake referenced this in commit 21ee676dd6 on Nov 14, 2019
  44. fanquake merged this on Nov 14, 2019
  45. fanquake closed this on Nov 14, 2019

  46. laanwj referenced this in commit 6ec0dc195d on Nov 14, 2019
  47. laanwj referenced this in commit 890dc0a7cc on Nov 14, 2019
  48. laanwj commented at 9:13 am on November 15, 2019: member
    Maybe you can get funding for such a thing from one of the exchanges or companies in the space, that’s always how developer funding has worked, but the ‘bitcoin core’ project is a loose name for people contributing to an open source project, and does not manage funds, and should not manage funds, and will never have an “official incentive structure”.
  49. practicalswift commented at 10:12 am on November 15, 2019: contributor

    […] the ‘bitcoin core’ project is a loose name for people contributing to an open source project, and does not manage funds, and should not manage funds, and will never have an “official incentive structure”.

    Agree 100% if we are talking monetary reward.

    Note that incentive structure does not necessarily imply monetary reward: giving proper credit and treating researchers nicely goes a surprisingly long way when it comes to attracting security research :)

    To bring this on-topic again (sorry, my fault!): I hope we can make this a good example by making sure @bitcoinVBR is properly credited for allowing us to ship 0.19.0 without an “optimisation unstable” UpdateTip(…) (due to the uninitialised read) :)

  50. laanwj commented at 10:38 am on November 15, 2019: member

    As all contributors to the release, they’ve been added to the authors list in 0.19.0.1 (as NullFunctor, as that’s the name on their git mail address, @bitcoinVBR: if you want to be credited under a different name let me know) and this PR has been added to the changelog.

    This is not a security issue so I’m not sure why you bring that up. It doesn’t matter either, bugs are bugs. All people that contribute deserve credit.

  51. fanquake commented at 1:41 pm on November 15, 2019: member
  52. fanquake removed the label Needs backport (0.19) on Nov 15, 2019
  53. Sjors commented at 2:18 pm on November 15, 2019: member
    ACK on the rebase 6ec0dc1.
  54. MarkLTZ referenced this in commit 3498cc6a56 on Nov 17, 2019
  55. domob1812 referenced this in commit 9b83b7de61 on Nov 18, 2019
  56. domob1812 referenced this in commit a4c95c6cc7 on Nov 18, 2019
  57. domob1812 referenced this in commit 5d00256d65 on Nov 18, 2019
  58. domob1812 referenced this in commit 1ba84a7cba on Nov 18, 2019
  59. fxtc referenced this in commit 5fa7246b82 on Nov 25, 2019
  60. fxtc referenced this in commit 31556e9abc on Nov 25, 2019
  61. fxtc referenced this in commit befa91ec37 on Nov 25, 2019
  62. fxtc referenced this in commit 7f5aa10721 on Nov 25, 2019
  63. ajtowns commented at 4:06 pm on November 26, 2019: member
    0consensus.MinBIP9WarningHeight = consensus.SegwitHeight + consensus.nMinerConfirmationWindow;
    

    Really fascinating that this 1.) wasn’t caught automatically by our static and dynamic analysis,

    Looks like -Werror=uninitialized is able to catch this, but only if the initialisation is done in a Consensus::Params constructor via a member initializer list rather than a code block (and -O3 is specified if using g++). (I thought I was getting g++ to catch it in code blocks as well, but can’t duplicate that now)

  64. practicalswift commented at 4:20 pm on November 26, 2019: contributor
    @ajtowns Worth mentioning from a dynamic analysis perspective is that both Valgrind and MemorySanitizer (-fsanitize=memory) catch this too :)
  65. kittywhiskers referenced this in commit 6718789ada on Dec 12, 2021
  66. MarcoFalke locked this on Dec 16, 2021

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-12-19 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me