refactor: Change * to & in MutableTransactionSignatureCreator #19426

The MutableTransactionSignatureCreator constructor takes in a pointer to a mutable transaction. This is problematic for several reasons:

• It would be undefined behaviour to pass in a nullptr because for signature creation, the memory of the mutable transaction is accessed
• No caller currently passes in a nullptr, so passing a reference as a pointer is confusing

Fix all issues by replacing * with & in MutableTransactionSignatureCreator

IIRC this was actually intentional (and maybe even in response to a review comment); it being a pointer makes it more clear that the object will store the passed-in argument, making it less likely that someone would accidentally pass in a temporary that would not outlive the constructed object.

Maybe this deserves the use of lifetimebound as introduced by #19387.

The nonnull attribute could be helpful as well, if this is kept as a pointer.

Concept ACK for making the implicit precondition (non-null mut-tx) explicit by the suggested change.

Concept super ACK for also adding lifetime annotations as suggested by @sipa to make the currently not entirely obvious lifetime hint explicit :)

Rationale in both cases:

• explicit is better than implicit
• compiler enforced is better than "try to remember"-enforced

See this list of similar cases that we may want to convert if reference + lifetimebound attribute is the direction we're going in: #19387 (comment)

<!--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:

• #22793 (Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors by achow101)
• #21702 (Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin)

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.

Maybe this deserves the use of lifetimebound

thx, done

Rebased

Closing for now due to lack of interest

Rebased

Code-review ACK fac6cfc50f65c610f2df9af3ec2efff5eade6661

Checked that the newly introduced [[clang::lifetimebound]] attribute for the first parameter applies by compiling with clang 13.0.0 and the following patch:

diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
index d41b54af2..dd995a445 100644
--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -361,6 +361,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
             UpdateInput(tx.vin[i], sigdata);
         }

+        auto mtxsig = MutableTransactionSignatureCreator(CMutableTransaction(), 0, 0, SIGHASH_ALL);
+
         // This should be valid under all script flags
         ValidateCheckInputsForAllFlags(CTransaction(tx), 0, true, m_node.chainman->ActiveChainstate().CoinsTip());

leading to the following warning:

  CXX      test/test_bitcoin-txvalidationcache_tests.o
test/txvalidationcache_tests.cpp:364:58: warning: temporary whose address is used as value of local variable 'mtxsig' will be destroyed at the end of the full-expression [-Wdangling]
        auto mtxsig = MutableTransactionSignatureCreator(CMutableTransaction(), 0, 0, SIGHASH_ALL);
                                                         ^~~~~~~~~~~~~~~~~~~~~
1 warning generated.

ACK fac6cfc50f65c610f2df9af3ec2efff5eade6661