Add defaults to vDeployments to avoid uninitialized variables #24032

pull ajtowns wants to merge 1 commits into bitcoin:master from ajtowns:202112-deployments changing 2 files +4 −3
  1. ajtowns commented at 5:57 AM on January 11, 2022: member

    Adds default values for vDeployments in consensus/params.h so that undefined behaviour is avoided if a deployment is not initialized. Also adds a check in the unit tests to alert if this is happening, since even if it doesn't invoke undefined behaviour it's probably a mistake.

  2. ajtowns commented at 6:02 AM on January 11, 2022: member

    Something like this was previously proposed in #21401. Deployments have been left uninitialised in the past, see #20157 and also taproot activation in elements so this seems worth ~having the compiler check for~ doing something to avoid similar mistakes. I think it ends up pretty readable too.

  3. ajtowns force-pushed on Jan 11, 2022
  4. DrahtBot commented at 6:19 AM on January 11, 2022: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  5. DrahtBot added the label Build system on Jan 11, 2022
  6. DrahtBot added the label Consensus on Jan 11, 2022
  7. DrahtBot added the label Mining on Jan 11, 2022
  8. DrahtBot added the label RPC/REST/ZMQ on Jan 11, 2022
  9. DrahtBot added the label Utils/log/libs on Jan 11, 2022
  10. DrahtBot added the label Validation on Jan 11, 2022
  11. in src/deploymentinfo.cpp:9 in 02bda4e42c outdated 
       5 | @@ -6,7 +6,7 @@
   6 |  
   7 |  #include <consensus/params.h>
   8 |  
   9 | -const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_BITS_DEPLOYMENTS] = {
  10 | +const struct std::array<VBDeploymentInfo, Consensus::MAX_VERSION_BITS_DEPLOYMENTS> VersionBitsDeploymentInfo{{
    MarcoFalke commented at 8:01 AM on January 11, 2022: 
    C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\deploymentinfo.cpp(9,109): error C2220: the following warning is treated as an error [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\deploymentinfo.cpp(9,109): warning C4099: 'std::array<VBDeploymentInfo,2>': type name first seen using 'class' now seen using 'struct' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\deploymentinfo.h(20): message : see declaration of 'std::array<VBDeploymentInfo,2>' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_common\libbitcoin_common.vcxproj]
    ajtowns commented at 8:50 AM on January 11, 2022:

    Huh, I can't get clang to duplicate that error, even with -Wmismatched-tags. Anyway, fixed.

  12. ajtowns force-pushed on Jan 11, 2022
  13. MarcoFalke removed the label Build system on Jan 11, 2022
  14. MarcoFalke removed the label RPC/REST/ZMQ on Jan 11, 2022
  15. MarcoFalke removed the label Mining on Jan 11, 2022
  16. MarcoFalke removed the label Validation on Jan 11, 2022
  17. MarcoFalke removed the label Utils/log/libs on Jan 11, 2022
  18. MarcoFalke added the label Refactoring on Jan 11, 2022
  19. in src/util/syntaxhelp.h:23 in 6eaf3afcbe outdated 
      18 | + *    bool _f(int x, int y) { ... }
  19 | + *
  20 | + * Then writing:
  21 | + *
  22 | + *    static constrexpr NamedArgument<int, struct x_arg> _x;
  23 | + *    static constrexpr NamedArgument<int, struct y_arg> _y;
    ajtowns commented at 3:44 AM on January 13, 2022:

    Typo: constrexpr has too many r's.

  20. in src/util/syntaxhelp.h:38 in 6eaf3afcbe outdated 
      33 | +{
  34 | +    struct Arg {
  35 | +        using Tag = TAG;
  36 | +        const T v;
  37 | +        Arg(T&& v) : v{std::move(v)} { }
  38 | +        Arg(const T& v) : v{v} { }
    ajtowns commented at 3:53 AM on January 13, 2022:

    Constructors should be explicit otherwise you can call as f(3, 4) instead of f(_x=3, _y=4).

  21. in src/util/syntaxhelp.h:65 in 6eaf3afcbe outdated 
      60 | +{
  61 | +    static constexpr size_t offset = O;
  62 | +    const T t;
  63 | +
  64 | +    template<typename... I>
  65 | +    ArrayInitElement(I... i) : t{i...} { };
    ajtowns commented at 3:58 AM on January 13, 2022:

    I think this potentially copies its arguments unnecessarily and would be better as ArrayInitElement(I&&... i) : t{std::forward<I>(i)...} { }. Also, no need for a semicolon.

  22. in src/util/syntaxhelp.h:88 in 6eaf3afcbe outdated 
      80 | +    template<size_t E, size_t O, typename... I>
  81 | +    static inline void step(Arr& a, const ArrayInitElement<O, T>& aie, const I&... i)
  82 | +    {
  83 | +        static_assert(E < N, "too many initializers for array");
  84 | +        static_assert(E == O, "array initializer element is out of order");
  85 | +        if constexpr (E < N) {
    ajtowns commented at 4:00 AM on January 13, 2022:

    FYI: this if constexpr is only here to avoid redundant warnings that std::get<N> and step<N+1> would generate.

  23. ajtowns commented at 4:02 AM on January 13, 2022: member

    Went over this with @amitiuttarwar and noticed some things.

  24. ajtowns force-pushed on Jan 13, 2022
  25. in src/chainparams.cpp:68 in 08aa93bf5f outdated 
      63 | +static constexpr util::NamedArgument<int64_t, struct start_time_arg> _start_time;
  64 | +static constexpr util::NamedArgument<int64_t, struct timeout_arg> _timeout;
  65 | +static constexpr util::NamedArgument<int, struct minactheight_arg> _minactheight;
  66 | +
  67 | +template<size_t O>
  68 | +static inline util::ArrayInitElement<O, Consensus::BIP9Deployment> Dep(decltype(_bit)::Arg bit, decltype(_start_time)::Arg start_time, decltype(_timeout)::Arg timeout, decltype(_minactheight)::Arg minactheight)
    PastaPastaPasta commented at 1:29 AM on January 14, 2022:

    I don't really like the name 'Dep'. Giving this a more descriptive name would, imo, be beneficial

    also, this may be more readable with trailing return ie

    static inline auto Dep(decltype(_bit)::Arg bit, decltype(_start_time)::Arg start_time, decltype(_timeout)::Arg timeout, decltype(_minactheight)::Arg minactheight) -> util::ArrayInitElement<O, Consensus::BIP9Deployment>
    ajtowns commented at 9:05 AM on January 15, 2022:

    Renamed to DepSettings

  26. in src/chainparams.cpp:67 in 08aa93bf5f outdated 
      62 | +static constexpr util::NamedArgument<int, struct bit_arg> _bit;
  63 | +static constexpr util::NamedArgument<int64_t, struct start_time_arg> _start_time;
  64 | +static constexpr util::NamedArgument<int64_t, struct timeout_arg> _timeout;
  65 | +static constexpr util::NamedArgument<int, struct minactheight_arg> _minactheight;
  66 | +
  67 | +template<size_t O>
    PastaPastaPasta commented at 1:31 AM on January 14, 2022:

    can you use a different name than O? I confused it for 0 when first reading

    ajtowns commented at 9:09 AM on January 15, 2022:

    Renamed to Offset; also, I renamed to Init

  27. in src/chainparams.cpp:75 in 08aa93bf5f outdated 
      70 | +    Consensus::BIP9Deployment dep;
  71 | +    dep.bit = bit.v;
  72 | +    dep.nStartTime = start_time.v;
  73 | +    dep.nTimeout = timeout.v;
  74 | +    dep.min_activation_height = minactheight.v;
  75 | +    return util::ArrayInitElement<O, Consensus::BIP9Deployment>{dep};
    PastaPastaPasta commented at 1:31 AM on January 14, 2022:

    Can you add a constructor to BIP9Deployment that takes these values? That way this method body collapses to just 1 line, and we'd then be passing a temporary

    ajtowns commented at 2:42 PM on January 14, 2022:

    gcc and clang both optimise them to the same thing, and saying dep.bit = bit.v etc seems a bit more obvious that everythings in the right order than BIP9Deployment(bit.v, start_time.v, timeout.v, minactheight.v) does. (per godbolt, VS seems to generate some moderately weird assembly, but changing how the temporary BIP9Deployment struct is created initialised doesn't seem to improve things)

  28. PastaPastaPasta commented at 1:33 AM on January 14, 2022: contributor

    Strong concept ACK, I have a few suggestions / concerns

  29. JeremyRubin commented at 2:53 AM on January 14, 2022: contributor

    Concept ACK

    is there other ongoing work on deployment tech that is motivating this change? Or just good maitenance.

  30. ajtowns commented at 2:57 PM on January 14, 2022: member

    is there other ongoing work on deployment tech that is motivating this change? Or just good maitenance.

    There were a bunch of clean ups that got dropped to get speedy trial done speedily; seems a good time to reconsider them now that versionbits logic isn't actively being used, and before we update to whatever might come after speedy trial. This seemed a good one to start with since it's caused bugs twice now.

  31. ajtowns force-pushed on Jan 15, 2022
  32. PastaPastaPasta commented at 6:39 AM on January 22, 2022: contributor

    Please see https://github.com/PastaPastaPasta/dash/commit/768a84f8ef1d544efd88ac9fae316df8777328fa to constexpr all the things

  33. ajtowns marked this as a draft on Jan 28, 2022
  34. ajtowns commented at 6:37 AM on January 28, 2022: member

    Marked as draft, since both #23508 (simple, user-visible improvement) and #23536 (changes how consensus is enforced so annoying to re-test after rebase) should be merged first, in my opinion.

  35. ajtowns force-pushed on Jan 28, 2022
  36. in src/versionbits.cpp:197 in 22886c1aac outdated 
     193 | @@ -194,15 +194,15 @@ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
 194 |  
 195 |  public:
 196 |      explicit VersionBitsConditionChecker(Consensus::DeploymentPos id_) : id(id_) {}
 197 | -    uint32_t Mask(const Consensus::Params& params) const { return ((uint32_t)1) << params.vDeployments[id].bit; }
 198 | +    uint32_t Mask(const Consensus::Params& params) const { return ((uint32_t)1) << params.vDeployments.at(id).bit; }
    PastaPastaPasta commented at 12:56 PM on February 1, 2022:

    please convert this to be a c++~11~ style functional cast, not c-style

    MarcoFalke commented at 1:09 PM on February 1, 2022:

    I don't think functional casts were added in C++11, pretty sure they existed before already.

    PastaPastaPasta commented at 3:21 PM on February 1, 2022:

    Yeah, seems like c++11 added the int{} syntax but not int(). Learn smth new everyday

    ajtowns commented at 8:36 PM on February 17, 2022:

    No longer touching this line

  37. ajtowns force-pushed on Feb 17, 2022
  38. ajtowns commented at 8:33 PM on February 17, 2022: member

    Rebased. Removed that .at() change based on https://www.erisian.com.au/bitcoin-core-dev/log-2022-02-02.html#l-301 . Still in draft for #23536

  39. ajtowns force-pushed on Mar 11, 2022
  40. ajtowns marked this as ready for review on Mar 11, 2022
  41. ajtowns commented at 10:12 AM on March 11, 2022: member

    Removes all the template magic; and downgrades the "you forgot to initialize a deployment" check from compile-time to the unit tests, and doesn't convert to std::array. On the upside, this means this PR no longer conflicts with #23536.

    ~Based on this week's meeting, also switches to using designated initializers.~ Doesn't do designated initializers because you seem to need to specify /std:c++latest instead of /std:c++17 for Visual Studio to support that.

  42. ajtowns force-pushed on Mar 11, 2022
  43. ajtowns renamed this:
    Refactor vDeployments setup to avoid uninitialized variables
    Add defaults to vDeployments to avoid uninitialized variables
    on Mar 11, 2022
  44. consensus/params: set default values for BIP9Deployment
    While chainparams should explicilty set values for each possible
entry in vDeployments, in the past that has been missed resulting
in potential undefined behaviour due to accessing unitinitialized
data. Reduce the severity of future bugs of that nature by providing
benign default values. Adds a unit test to alert if the default value
is not overwritten for the real chains (NEVER_ACTIVE/NEVER_ACTIVE rather
than NEVER_ACTIVE/NO_TIMEOUT).
    c4c5b9ca6e
  45. ajtowns force-pushed on Apr 5, 2022
  46. laanwj added this to the "Blockers" column in a project
  47. laanwj commented at 5:34 PM on May 26, 2022: member

    Code review ACK c4c5b9ca6e98cf44309af73edf5559940a04d00f

  48. laanwj merged this on May 26, 2022
  49. laanwj closed this on May 26, 2022
  50. laanwj removed this from the "Blockers" column in a project
  51. sidhujag referenced this in commit bee2c22bcb on May 28, 2022
  52. DrahtBot locked this on May 26, 2023
Contributors
ajtownsDrahtBotMarcoFalkePastaPastaPastaJeremyRubinlaanwj
Labels
RefactoringConsensus
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-04-22 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me