refactor: Remove confusing P1008R1 violation in ATMPArgs

[maflcko]

The = delete doesn't achieve the stated goal and it is also redundant, since it is not possible to default construct the ATMPArgs type.

This can be tested with:

diff --git a/src/validation.cpp b/src/validation.cpp
index 2813b62462..1c939c0b8a 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -519,6 +519,7 @@ public:
         /** Parameters for child-with-unconfirmed-parents package validation. */
         static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time,
                                                 std::vector<COutPoint>& coins_to_uncache) {
+            ATMPArgs{};
             return ATMPArgs{/* m_chainparams */ chainparams,
                             /* m_accept_time */ accept_time,
                             /* m_bypass_limits */ false,

Which fails on current master and this pull with the following error:

validation.cpp:525:22: error: reference member of type 'const CChainParams &' uninitialized
            ATMPArgs{};
                    ~^
validation.cpp:470:29: note: uninitialized reference member is here
        const CChainParams& m_chainparams;
                            ^
1 error generated.

Further reading (optional):

• http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf

[maflcko]

I believe an alternative (and larger) fix would be to provide an explicit constructor that takes all members as arguments. Happy to switch to that, but for now I prefer the smaller patch.

[glozow]

ACK facbbec

I believe an alternative (and larger) fix would be to provide an explicit constructor that takes all members as arguments.

I prefer this as well, for other places where there's no uninitialized reference to take the bullet for us.

[glozow]

CI error is the assert_fee_amount thing, unrelated

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #24169 (build: Add --enable-c++20 option by MarcoFalke)
• #24152 (policy / validation: CPFP fee bumping within packages by glozow)

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.

[JeremyRubin]

NACK.

The point of a deleted constructor is to make the type stable to future modifications that might e.g. turn m_chainparams into p_chainparams. It's essentially an assertion.

[maflcko]

It's essentially an assertion.

No, it is not. Even if no member was const, aggregate initialization is possible. On current master and this branch. This patch isn't changing what it possible.

[maflcko]

If you don't believe me, nor the paper P1008R1. You can try it yourself with a C++11 compiler of you choice. The following compiles fine with or without the delete.

struct ATMPArgs {
  int a;
  bool b;
  ATMPArgs() = delete;
};
int main() { (void)ATMPArgs{}; } // compiles

[glozow]

The point of a deleted constructor is to make the type stable to future modifications that might e.g. turn m_chainparams into p_chainparams. It's essentially an assertion.

No that was not the point when writing this code. The intention was to prevent callers from specifying their own ATMPArgs, e.g. using ATMPArgs args(); args.m_bypass_limits = true;. I wanted them to only be able to initialize arguments using the static constructors (ATMPArgs::SingleAccept, ATMPArgs::TestPackageAccept).

But as shown in the paper (and any compiler), deleting the default constructor doesn't prevent something like this. What does prevent this is that there's a reference in the struct so you can't create a default version of it. So deletion of the default constructor does nothing. This PR is essentially removing code that doesn't do anything and misleading/confusing documentation.

[hebasto]

This example looks scary:

struct ATMPArgs {
  int a = 42;
  bool b;
  ATMPArgs() = delete;
  ATMPArgs(int) = delete;
};

int main()
{
    constexpr ATMPArgs atm{0};
    static_assert(atm.a == 0);
}

[achow101]

I think it's also important to point out that P1008R1 was accepted into C++20 so keeping the delete would also remove the aggregate initialization that the static functions make use of to construct ATMPArgs. However, I think that it is not immediately obvious that the default constructor is deleted by having a reference as a member. I think it would still be useful to comment out the delete and include some additional comments as to why that is.

[JeremyRubin]

The workaround in P1008R1 is to add an explicit modifier to the deletions? Why is that not a workable option here?

[maflcko]

Why is that not a workable option here?

It wouldn't compile:

validation.cpp:522:20: error: no matching constructor for initialization of '(anonymous namespace)::MemPoolAccept::ATMPArgs'
            return ATMPArgs{/* m_chainparams */ chainparams,
                   ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
validation.cpp:469:12: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 7 were provided
    struct ATMPArgs {
           ^
validation.cpp:469:12: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 7 were provided
validation.cpp:533:18: note: candidate constructor not viable: requires 0 arguments, but 7 were provided
        explicit ATMPArgs() = delete;
                 ^
3 errors generated.

with diff on master:

diff --git a/src/validation.cpp b/src/validation.cpp
index 035b5783c3..c714bfe575 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -530,7 +530,7 @@ public:
         }
         // No default ctor to avoid exposing details to clients and allowing the possibility of
         // mixing up the order of the arguments. Use static functions above instead.
-        ATMPArgs() = delete;
+        explicit ATMPArgs() = delete;
     };
 
     // Single transaction acceptance

[maflcko]

I've pushed the alternative fix I mentioned in comment 2 #24404 (comment)

[maflcko]

Suggested diff for testing:

diff --git a/src/validation.cpp b/src/validation.cpp
index 3f64fcc067..900f85fc54 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -707,6 +707,14 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     if (tx.IsCoinBase())
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "coinbase");
 
+            ATMPArgs{       args.m_chainparams,
+                            args.m_accept_time,
+                            args.m_bypass_limits,
+                            args.m_coins_to_uncache,
+                            args.m_test_accept,
+                            args.m_allow_bip125_replacement,
+                            args.m_package_submission
+            };
     // Rather not work on nonstandard transactions (unless -testnet/-regtest)
     std::string reason;
     if (fRequireStandard && !IsStandardTx(tx, reason))

[JeremyRubin]

should you not also retain the explicit delete with a new explicit ctor?

[maflcko]

Happy to do so if there is any benefit?

[maflcko]

Tend toward leaving as-is. explicit isn't used in current master and it isn't used in this pull. If there is any reason to use it, it might be best to do it in a separate pull request (as a follow-up or conflicting pull), with proper motivation.

[glozow]

code review ACK faa1aec

[achow101]

ACK faa1aec