The MutableTransactionSignatureCreator constructor takes in a pointer to a mutable transaction. This is problematic for several reasons:
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.
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:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Maybe this deserves the use of lifetimebound
thx, done
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:
0diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
1index d41b54af2..dd995a445 100644
2--- a/src/test/txvalidationcache_tests.cpp
3+++ b/src/test/txvalidationcache_tests.cpp
4@@ -361,6 +361,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
5 UpdateInput(tx.vin[i], sigdata);
6 }
7
8+ auto mtxsig = MutableTransactionSignatureCreator(CMutableTransaction(), 0, 0, SIGHASH_ALL);
9+
10 // This should be valid under all script flags
11 ValidateCheckInputsForAllFlags(CTransaction(tx), 0, true, m_node.chainman->ActiveChainstate().CoinsTip());
leading to the following warning:
0 CXX test/test_bitcoin-txvalidationcache_tests.o
1test/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]
2 auto mtxsig = MutableTransactionSignatureCreator(CMutableTransaction(), 0, 0, SIGHASH_ALL);
3 ^~~~~~~~~~~~~~~~~~~~~
41 warning generated.
