-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Change * to & in MutableTransactionSignatureCreator #19426
Conversation
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 |
The |
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:
|
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) |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
fa2bd85
to
fac6c14
Compare
thx, done |
fac6c14
to
86b2d8e
Compare
Rebased |
Closing for now due to lack of interest |
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK fac6cfc
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fac6cfc
…natureCreator fac6cfc refactor: Change * to & in MutableTransactionSignatureCreator (MarcoFalke) Pull request description: 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` ACKs for top commit: theStack: Code-review ACK fac6cfc jonatack: ACK fac6cfc Tree-SHA512: d84296b030bd4fa2709e5adbfe43a5f8377d218957d844af69a819893252af671df7f00004f5ba601a0bd70f3c1c2e58c4f00e75684da663f28432bb5c89fb86
The
MutableTransactionSignatureCreator
constructor takes in a pointer to a mutable transaction. This is problematic for several reasons:Fix all issues by replacing
*
with&
inMutableTransactionSignatureCreator