Skip to content

Commit b557a24

Browse files
author
MacroFake
committed
Merge bitcoin/bitcoin#19426: refactor: Change * to & in MutableTransactionSignatureCreator
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
2 parents b2e7811 + fac6cfc commit b557a24

File tree

10 files changed

+28
-26
lines changed

10 files changed

+28
-26
lines changed

‎src/bitcoin-tx.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -667,7 +667,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
667667
SignatureData sigdata = DataFromTransaction(mergedTx, i, coin.out);
668668
// Only sign SIGHASH_SINGLE if there's a corresponding output:
669669
if (!fHashSingle || (i < mergedTx.vout.size()))
670-
ProduceSignature(keystore, MutableTransactionSignatureCreator(&mergedTx, i, amount, nHashType), prevPubKey, sigdata);
670+
ProduceSignature(keystore, MutableTransactionSignatureCreator(mergedTx, i, amount, nHashType), prevPubKey, sigdata);
671671

672672
if (amount == MAX_MONEY && !sigdata.scriptWitness.IsNull()) {
673673
throw std::runtime_error(strprintf("Missing amount for CTxOut with scriptPubKey=%s", HexStr(prevPubKey)));

‎src/psbt.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransactio
234234
// Construct a would-be spend of this output, to update sigdata with.
235235
// Note that ProduceSignature is used to fill in metadata (not actual signatures),
236236
// so provider does not need to provide any private keys (it can be a HidingSigningProvider).
237-
MutableTransactionSignatureCreator creator(&tx, /*input_idx=*/0, out.nValue, SIGHASH_ALL);
237+
MutableTransactionSignatureCreator creator(tx, /*input_idx=*/0, out.nValue, SIGHASH_ALL);
238238
ProduceSignature(provider, creator, out.scriptPubKey, sigdata);
239239

240240
// Put redeem_script, witness_script, key paths, into PSBTOutput.
@@ -301,7 +301,7 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
301301
if (txdata == nullptr) {
302302
sig_complete = ProduceSignature(provider, DUMMY_SIGNATURE_CREATOR, utxo.scriptPubKey, sigdata);
303303
} else {
304-
MutableTransactionSignatureCreator creator(&tx, index, utxo.nValue, txdata, sighash);
304+
MutableTransactionSignatureCreator creator(tx, index, utxo.nValue, txdata, sighash);
305305
sig_complete = ProduceSignature(provider, creator, utxo.scriptPubKey, sigdata);
306306
}
307307
// Verify that a witness signature was produced in case one was required.

‎src/rpc/rawtransaction.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,7 @@ static RPCHelpMan combinerawtransaction()
567567
sigdata.MergeSignatureData(DataFromTransaction(txv, i, coin.out));
568568
}
569569
}
570-
ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&mergedTx, i, coin.out.nValue, 1), coin.out.scriptPubKey, sigdata);
570+
ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(mergedTx, i, coin.out.nValue, 1), coin.out.scriptPubKey, sigdata);
571571

572572
UpdateInput(txin, sigdata);
573573
}

‎src/script/sign.cpp

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,16 @@
1818

1919
typedef std::vector<unsigned char> valtype;
2020

21-
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, int hash_type)
22-
: txTo{tx}, nIn{input_idx}, nHashType{hash_type}, amount{amount}, checker{txTo, nIn, amount, MissingDataBehavior::FAIL},
21+
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction& tx, unsigned int input_idx, const CAmount& amount, int hash_type)
22+
: m_txto{tx}, nIn{input_idx}, nHashType{hash_type}, amount{amount}, checker{&m_txto, nIn, amount, MissingDataBehavior::FAIL},
2323
m_txdata(nullptr)
2424
{
2525
}
2626

27-
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type)
28-
: txTo{tx}, nIn{input_idx}, nHashType{hash_type}, amount{amount},
29-
checker{txdata ? MutableTransactionSignatureChecker{txTo, nIn, amount, *txdata, MissingDataBehavior::FAIL} :
30-
MutableTransactionSignatureChecker{txTo, nIn, amount, MissingDataBehavior::FAIL}},
27+
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction& tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type)
28+
: m_txto{tx}, nIn{input_idx}, nHashType{hash_type}, amount{amount},
29+
checker{txdata ? MutableTransactionSignatureChecker{&m_txto, nIn, amount, *txdata, MissingDataBehavior::FAIL} :
30+
MutableTransactionSignatureChecker{&m_txto, nIn, amount, MissingDataBehavior::FAIL}},
3131
m_txdata(txdata)
3232
{
3333
}
@@ -50,7 +50,7 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid
5050
// BASE/WITNESS_V0 signatures don't support explicit SIGHASH_DEFAULT, use SIGHASH_ALL instead.
5151
const int hashtype = nHashType == SIGHASH_DEFAULT ? SIGHASH_ALL : nHashType;
5252

53-
uint256 hash = SignatureHash(scriptCode, *txTo, nIn, hashtype, amount, sigversion, m_txdata);
53+
uint256 hash = SignatureHash(scriptCode, m_txto, nIn, hashtype, amount, sigversion, m_txdata);
5454
if (!key.Sign(hash, vchSig))
5555
return false;
5656
vchSig.push_back((unsigned char)hashtype);
@@ -80,7 +80,7 @@ bool MutableTransactionSignatureCreator::CreateSchnorrSig(const SigningProvider&
8080
execdata.m_tapleaf_hash = *leaf_hash;
8181
}
8282
uint256 hash;
83-
if (!SignatureHashSchnorr(hash, execdata, *txTo, nIn, nHashType, sigversion, *m_txdata, MissingDataBehavior::FAIL)) return false;
83+
if (!SignatureHashSchnorr(hash, execdata, m_txto, nIn, nHashType, sigversion, *m_txdata, MissingDataBehavior::FAIL)) return false;
8484
sig.resize(64);
8585
// Use uint256{} as aux_rnd for now.
8686
if (!key.SignSchnorr(hash, sig, merkle_root, {})) return false;
@@ -540,7 +540,7 @@ bool SignSignature(const SigningProvider &provider, const CScript& fromPubKey, C
540540
{
541541
assert(nIn < txTo.vin.size());
542542

543-
MutableTransactionSignatureCreator creator(&txTo, nIn, amount, nHashType);
543+
MutableTransactionSignatureCreator creator(txTo, nIn, amount, nHashType);
544544

545545
SignatureData sigdata;
546546
bool ret = ProduceSignature(provider, creator, fromPubKey, sigdata);
@@ -679,7 +679,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
679679
SignatureData sigdata = DataFromTransaction(mtx, i, coin->second.out);
680680
// Only sign SIGHASH_SINGLE if there's a corresponding output:
681681
if (!fHashSingle || (i < mtx.vout.size())) {
682-
ProduceSignature(*keystore, MutableTransactionSignatureCreator(&mtx, i, amount, &txdata, nHashType), prevPubKey, sigdata);
682+
ProduceSignature(*keystore, MutableTransactionSignatureCreator(mtx, i, amount, &txdata, nHashType), prevPubKey, sigdata);
683683
}
684684

685685
UpdateInput(txin, sigdata);

‎src/script/sign.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#ifndef BITCOIN_SCRIPT_SIGN_H
77
#define BITCOIN_SCRIPT_SIGN_H
88

9+
#include <attributes.h>
910
#include <coins.h>
1011
#include <hash.h>
1112
#include <pubkey.h>
@@ -34,17 +35,18 @@ class BaseSignatureCreator {
3435
};
3536

3637
/** A signature creator for transactions. */
37-
class MutableTransactionSignatureCreator : public BaseSignatureCreator {
38-
const CMutableTransaction* txTo;
38+
class MutableTransactionSignatureCreator : public BaseSignatureCreator
39+
{
40+
const CMutableTransaction& m_txto;
3941
unsigned int nIn;
4042
int nHashType;
4143
CAmount amount;
4244
const MutableTransactionSignatureChecker checker;
4345
const PrecomputedTransactionData* m_txdata;
4446

4547
public:
46-
MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, int hash_type);
47-
MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type);
48+
MutableTransactionSignatureCreator(const CMutableTransaction& tx LIFETIMEBOUND, unsigned int input_idx, const CAmount& amount, int hash_type);
49+
MutableTransactionSignatureCreator(const CMutableTransaction& tx LIFETIMEBOUND, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type);
4850
const BaseSignatureChecker& Checker() const override { return checker; }
4951
bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override;
5052
bool CreateSchnorrSig(const SigningProvider& provider, std::vector<unsigned char>& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const override;

‎src/test/descriptor_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ void DoCheck(const std::string& prv, const std::string& pub, const std::string&
312312
std::vector<CTxOut> utxos(1);
313313
PrecomputedTransactionData txdata;
314314
txdata.Init(spend, std::move(utxos), /*force=*/true);
315-
MutableTransactionSignatureCreator creator(&spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT);
315+
MutableTransactionSignatureCreator creator{spend, 0, CAmount{0}, &txdata, SIGHASH_DEFAULT};
316316
SignatureData sigdata;
317317
BOOST_CHECK_MESSAGE(ProduceSignature(Merge(keys_priv, script_provider), creator, spks[n], sigdata), prv);
318318
}

‎src/test/fuzz/script_sign.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ FUZZ_TARGET_INIT(script_sign, initialize_script_sign)
113113
}
114114
if (n_in < script_tx_to.vin.size()) {
115115
(void)SignSignature(provider, ConsumeScript(fuzzed_data_provider), script_tx_to, n_in, ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<int>());
116-
MutableTransactionSignatureCreator signature_creator{&tx_to, n_in, ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<int>()};
116+
MutableTransactionSignatureCreator signature_creator{tx_to, n_in, ConsumeMoney(fuzzed_data_provider), fuzzed_data_provider.ConsumeIntegral<int>()};
117117
std::vector<unsigned char> vch_sig;
118118
CKeyID address;
119119
if (fuzzed_data_provider.ConsumeBool()) {

‎src/test/script_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1162,7 +1162,7 @@ SignatureData CombineSignatures(const CTxOut& txout, const CMutableTransaction&
11621162
SignatureData data;
11631163
data.MergeSignatureData(scriptSig1);
11641164
data.MergeSignatureData(scriptSig2);
1165-
ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&tx, 0, txout.nValue, SIGHASH_DEFAULT), txout.scriptPubKey, data);
1165+
ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(tx, 0, txout.nValue, SIGHASH_DEFAULT), txout.scriptPubKey, data);
11661166
return data;
11671167
}
11681168

@@ -1796,7 +1796,7 @@ BOOST_AUTO_TEST_CASE(bip341_keypath_test_vectors)
17961796
// Sign and verify signature.
17971797
FlatSigningProvider provider;
17981798
provider.keys[key.GetPubKey().GetID()] = key;
1799-
MutableTransactionSignatureCreator creator(&tx, txinpos, utxos[txinpos].nValue, &txdata, hashtype);
1799+
MutableTransactionSignatureCreator creator(tx, txinpos, utxos[txinpos].nValue, &txdata, hashtype);
18001800
std::vector<unsigned char> signature;
18011801
BOOST_CHECK(creator.CreateSchnorrSig(provider, signature, pubkey, nullptr, &merkle_root, SigVersion::TAPROOT));
18021802
BOOST_CHECK_EQUAL(HexStr(signature), input["expected"]["witness"][0].get_str());

‎src/test/transaction_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ SignatureData CombineSignatures(const CMutableTransaction& input1, const CMutabl
559559
SignatureData sigdata;
560560
sigdata = DataFromTransaction(input1, 0, tx->vout[0]);
561561
sigdata.MergeSignatureData(DataFromTransaction(input2, 0, tx->vout[0]));
562-
ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&input1, 0, tx->vout[0].nValue, SIGHASH_ALL), tx->vout[0].scriptPubKey, sigdata);
562+
ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(input1, 0, tx->vout[0].nValue, SIGHASH_ALL), tx->vout[0].scriptPubKey, sigdata);
563563
return sigdata;
564564
}
565565

‎src/test/txvalidationcache_tests.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
329329

330330
// Sign
331331
SignatureData sigdata;
332-
BOOST_CHECK(ProduceSignature(keystore, MutableTransactionSignatureCreator(&valid_with_witness_tx, 0, 11*CENT, SIGHASH_ALL), spend_tx.vout[1].scriptPubKey, sigdata));
332+
BOOST_CHECK(ProduceSignature(keystore, MutableTransactionSignatureCreator(valid_with_witness_tx, 0, 11 * CENT, SIGHASH_ALL), spend_tx.vout[1].scriptPubKey, sigdata));
333333
UpdateInput(valid_with_witness_tx.vin[0], sigdata);
334334

335335
// This should be valid under all script flags.
@@ -355,9 +355,9 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
355355
tx.vout[0].scriptPubKey = p2pk_scriptPubKey;
356356

357357
// Sign
358-
for (int i=0; i<2; ++i) {
358+
for (int i = 0; i < 2; ++i) {
359359
SignatureData sigdata;
360-
BOOST_CHECK(ProduceSignature(keystore, MutableTransactionSignatureCreator(&tx, i, 11*CENT, SIGHASH_ALL), spend_tx.vout[i].scriptPubKey, sigdata));
360+
BOOST_CHECK(ProduceSignature(keystore, MutableTransactionSignatureCreator(tx, i, 11 * CENT, SIGHASH_ALL), spend_tx.vout[i].scriptPubKey, sigdata));
361361
UpdateInput(tx.vin[i], sigdata);
362362
}
363363

0 commit comments

Comments
 (0)