Skip to content
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.

Sign up for GitHub

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

Backport changes in TX verification/signing API #3101

Merged
merged 6 commits into from
Sep 22, 2019
Merged

Backport changes in TX verification/signing API #3101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8da543f
Refactor script validation to observe amounts
sipa Mar 31, 2016
fff708d
BIP143: Verification logic
sipa Dec 27, 2015
21eca64
BIP143: Signing logic
sipa Mar 31, 2016
16bd20e
Remove segwit related code
codablock Sep 19, 2019
7662c0b
A couple of fixes
codablock Sep 19, 2019
689eff3
Drop unused GetScriptForWitness
codablock Sep 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions src/dash-tx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,18 @@ static bool findSighashFlags(int& flags, const std::string& flagStr)
return false;
}

static CAmount AmountFromValue(const UniValue& value)
{
if (!value.isNum() && !value.isStr())
throw std::runtime_error("Amount is not a number or string");
CAmount amount;
if (!ParseFixedPoint(value.getValStr(), 8, &amount))
throw std::runtime_error("Invalid amount");
if (!MoneyRange(amount))
throw std::runtime_error("Amount out of range");
return amount;
}

static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
{
int nHashType = SIGHASH_ALL;
Expand Down Expand Up @@ -548,6 +560,9 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
Coin newcoin;
newcoin.out.scriptPubKey = scriptPubKey;
newcoin.out.nValue = 0; // we don't know the actual output value
if (prevOut.exists("amount")) {
newcoin.out.nValue = AmountFromValue(prevOut["amount"]);
}
newcoin.nHeight = 1;
view.AddCoin(out, std::move(newcoin), true);
}
Expand Down Expand Up @@ -577,17 +592,18 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
continue;
}
const CScript& prevPubKey = coin.out.scriptPubKey;
const CAmount& amount = coin.out.nValue;

txin.scriptSig.clear();
SignatureData sigdata;
// Only sign SIGHASH_SINGLE if there's a corresponding output:
if (!fHashSingle || (i < mergedTx.vout.size()))
SignSignature(keystore, prevPubKey, mergedTx, i, nHashType);
ProduceSignature(MutableTransactionSignatureCreator(&keystore, &mergedTx, i, amount, nHashType), prevPubKey, sigdata);

// ... and merge in other signatures:
for (const CTransaction& txv : txVariants) {
txin.scriptSig = CombineSignatures(prevPubKey, mergedTx, i, txin.scriptSig, txv.vin[i].scriptSig);
}
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker(&mergedTx, i)))
for (const CTransaction& txv : txVariants)
sigdata = CombineSignatures(prevPubKey, MutableTransactionSignatureChecker(&mergedTx, i, amount), sigdata, DataFromTransaction(txv, i));
UpdateTransaction(mergedTx, i, sigdata);
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker(&mergedTx, i, amount)))
fComplete = false;
}

Expand Down
2 changes: 1 addition & 1 deletion src/policy/policy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
{
std::vector<std::vector<unsigned char> > stack;
// convert the scriptSig into a stack, so we can inspect the redeemScript
if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), 0))
if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SIGVERSION_BASE))
return false;
if (stack.empty())
return false;
Expand Down
3 changes: 2 additions & 1 deletion src/privatesend/privatesend-client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,8 @@ bool CPrivateSendClientSession::SignFinalTransaction(const CTransaction& finalTr
const CKeyStore& keystore = *vpwallets[0];

LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- Signing my input %i\n", nMyInputIndex);
if (!SignSignature(keystore, prevPubKey, finalMutableTransaction, nMyInputIndex, int(SIGHASH_ALL | SIGHASH_ANYONECANPAY))) { // changes scriptSig
// TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit)
if (!SignSignature(keystore, prevPubKey, finalMutableTransaction, nMyInputIndex, 0, int(SIGHASH_ALL | SIGHASH_ANYONECANPAY))) { // changes scriptSig
LogPrint(BCLog::PRIVATESEND, "CPrivateSendClientSession::SignFinalTransaction -- Unable to sign my own transaction!\n");
// not sure what to do here, it will timeout...?
}
Expand Down
3 changes: 2 additions & 1 deletion src/privatesend/privatesend-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,8 @@ bool CPrivateSendServer::IsInputScriptSigValid(const CTxIn& txin)
if (nTxInIndex >= 0) { //might have to do this one input at a time?
txNew.vin[nTxInIndex].scriptSig = txin.scriptSig;
LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::IsInputScriptSigValid -- verifying scriptSig %s\n", ScriptToAsmStr(txin.scriptSig).substr(0, 24));
if (!VerifyScript(txNew.vin[nTxInIndex].scriptSig, sigPubKey, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, MutableTransactionSignatureChecker(&txNew, nTxInIndex))) {
// TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this just mean calculating the amount from the txIns above and adding that here?

We won't actually ever use this even then, right?

Copy link

@UdjinM6 UdjinM6 Sep 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never say never :D Signing the input amount could be very useful (e.g. for HW wallets https://blog.trezor.io/what-segregated-witness-means-for-trezor-808c790a05bd), imo we should consider adding this feature one day.

As for TODO part - we could just use txNew.vout[0].nValue here because we know all inputs and outputs MUST be of the same value, this is verified earlier and we should not get here if this is not the case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yepp agree with @UdjinM6 about never saying never ;)
The "good" thing here is, if we ever start to use the amount while signing, it will break PS code and then this TODO comment will give the hint on what needs to be fixed :D

if (!VerifyScript(txNew.vin[nTxInIndex].scriptSig, sigPubKey, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, MutableTransactionSignatureChecker(&txNew, nTxInIndex, 0))) {
LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::IsInputScriptSigValid -- VerifyScript() failed on input %d\n", nTxInIndex);
return false;
}
Expand Down
18 changes: 13 additions & 5 deletions src/rpc/rawtransaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,8 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
" \"txid\":\"id\", (string, required) The transaction id\n"
" \"vout\":n, (numeric, required) The output number\n"
" \"scriptPubKey\": \"hex\", (string, required) script key\n"
" \"redeemScript\": \"hex\" (string, required for P2SH) redeem script\n"
" \"redeemScript\": \"hex\", (string, required for P2SH) redeem script\n"
" \"amount\": value (numeric, required) The amount spent\n"
" }\n"
" ,...\n"
" ]\n"
Expand Down Expand Up @@ -754,6 +755,9 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
Coin newcoin;
newcoin.out.scriptPubKey = scriptPubKey;
newcoin.out.nValue = 0;
if (prevOut.exists("amount")) {
newcoin.out.nValue = AmountFromValue(find_value(prevOut, "amount"));
}
newcoin.nHeight = 1;
view.AddCoin(out, std::move(newcoin), true);
}
Expand Down Expand Up @@ -818,20 +822,24 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
continue;
}
const CScript& prevPubKey = coin.out.scriptPubKey;
const CAmount& amount = coin.out.nValue;

txin.scriptSig.clear();
SignatureData sigdata;
// Only sign SIGHASH_SINGLE if there's a corresponding output:
if (!fHashSingle || (i < mergedTx.vout.size()))
SignSignature(keystore, prevPubKey, mergedTx, i, nHashType);
ProduceSignature(MutableTransactionSignatureCreator(&keystore, &mergedTx, i, amount, nHashType), prevPubKey, sigdata);

// ... and merge in other signatures:
for (const CMutableTransaction& txv : txVariants) {
if (txv.vin.size() > i) {
txin.scriptSig = CombineSignatures(prevPubKey, txConst, i, txin.scriptSig, txv.vin[i].scriptSig);
sigdata = CombineSignatures(prevPubKey, TransactionSignatureChecker(&txConst, i, amount), sigdata, DataFromTransaction(txv, i));
}
}

UpdateTransaction(mergedTx, i, sigdata);

ScriptError serror = SCRIPT_ERR_OK;
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i), &serror)) {
if (!VerifyScript(txin.scriptSig, prevPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount), &serror)) {
TxInErrorToJSON(txin, vErrors, ScriptErrorString(serror));
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/script/dashconsensus.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ int dashconsensus_verify_script(const unsigned char *scriptPubKey, unsigned int
set_error(err, dashconsensus_ERR_OK);

PrecomputedTransactionData txdata(tx);
return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), flags, TransactionSignatureChecker(&tx, nIn, txdata), nullptr);
CAmount am(0);
return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), flags, TransactionSignatureChecker(&tx, nIn, am, txdata), nullptr);
} catch (const std::exception&) {
return set_error(err, dashconsensus_ERR_TX_DESERIALIZE); // Error deserializing
}
Expand Down
2 changes: 2 additions & 0 deletions src/script/dashconsensus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#ifndef BITCOIN_BITCOINCONSENSUS_H
#define BITCOIN_BITCOINCONSENSUS_H

#include <stdint.h>

#if defined(BUILD_BITCOIN_INTERNAL) && defined(HAVE_CONFIG_H)
#include "config/dash-config.h"
#if defined(_WIN32)
Expand Down
26 changes: 15 additions & 11 deletions src/script/interpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ bool static CheckMinimalPush(const valtype& data, opcodetype opcode) {
return true;
}

bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror)
bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror)
{
static const CScriptNum bnZero(0);
static const CScriptNum bnOne(1);
Expand Down Expand Up @@ -867,13 +867,15 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
CScript scriptCode(pbegincodehash, pend);

// Drop the signature, since there's no way for a signature to sign itself
scriptCode.FindAndDelete(CScript(vchSig));
if (sigversion == SIGVERSION_BASE) {
scriptCode.FindAndDelete(CScript(vchSig));
}

if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
//serror is set
return false;
}
bool fSuccess = checker.CheckSig(vchSig, vchPubKey, scriptCode);
bool fSuccess = checker.CheckSig(vchSig, vchPubKey, scriptCode, sigversion);

if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && vchSig.size())
return set_error(serror, SCRIPT_ERR_SIG_NULLFAIL);
Expand Down Expand Up @@ -929,7 +931,9 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
for (int k = 0; k < nSigsCount; k++)
{
valtype& vchSig = stacktop(-isig-k);
scriptCode.FindAndDelete(CScript(vchSig));
if (sigversion == SIGVERSION_BASE) {
scriptCode.FindAndDelete(CScript(vchSig));
}
}

bool fSuccess = true;
Expand All @@ -947,7 +951,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
}

// Check signature
bool fOk = checker.CheckSig(vchSig, vchPubKey, scriptCode);
bool fOk = checker.CheckSig(vchSig, vchPubKey, scriptCode, sigversion);

if (fOk) {
isig++;
Expand Down Expand Up @@ -1150,7 +1154,7 @@ PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction& txTo)
hashOutputs = GetOutputsHash(txTo);
}

uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const PrecomputedTransactionData* cache)
uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache)
{
static const uint256 one(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));
if (nIn >= txTo.vin.size()) {
Expand Down Expand Up @@ -1180,7 +1184,7 @@ bool TransactionSignatureChecker::VerifySignature(const std::vector<unsigned cha
return pubkey.Verify(sighash, vchSig);
}

bool TransactionSignatureChecker::CheckSig(const std::vector<unsigned char>& vchSigIn, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode) const
bool TransactionSignatureChecker::CheckSig(const std::vector<unsigned char>& vchSigIn, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
{
CPubKey pubkey(vchPubKey);
if (!pubkey.IsValid())
Expand All @@ -1193,7 +1197,7 @@ bool TransactionSignatureChecker::CheckSig(const std::vector<unsigned char>& vch
int nHashType = vchSig.back();
vchSig.pop_back();

uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, this->txdata);
uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata);

if (!VerifySignature(vchSig, pubkey, sighash))
return false;
Expand Down Expand Up @@ -1292,12 +1296,12 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigne
}

std::vector<std::vector<unsigned char> > stack, stackCopy;
if (!EvalScript(stack, scriptSig, flags, checker, serror))
if (!EvalScript(stack, scriptSig, flags, checker, SIGVERSION_BASE, serror))
// serror is set
return false;
if (flags & SCRIPT_VERIFY_P2SH)
stackCopy = stack;
if (!EvalScript(stack, scriptPubKey, flags, checker, serror))
if (!EvalScript(stack, scriptPubKey, flags, checker, SIGVERSION_BASE, serror))
// serror is set
return false;
if (stack.empty())
Expand All @@ -1324,7 +1328,7 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigne
CScript pubKey2(pubKeySerialized.begin(), pubKeySerialized.end());
popstack(stack);

if (!EvalScript(stack, pubKey2, flags, checker, serror))
if (!EvalScript(stack, pubKey2, flags, checker, SIGVERSION_BASE, serror))
// serror is set
return false;
if (stack.empty())
Expand Down
20 changes: 13 additions & 7 deletions src/script/interpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,17 @@ struct PrecomputedTransactionData
PrecomputedTransactionData(const CTransaction& tx);
};

uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const PrecomputedTransactionData* cache = nullptr);
enum SigVersion
{
SIGVERSION_BASE = 0,
};

uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr);

class BaseSignatureChecker
{
public:
virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode) const
virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
{
return false;
}
Expand All @@ -129,15 +134,16 @@ class TransactionSignatureChecker : public BaseSignatureChecker
private:
const CTransaction* txTo;
unsigned int nIn;
const CAmount amount;
const PrecomputedTransactionData* txdata;

protected:
virtual bool VerifySignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const;

public:
TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn) : txTo(txToIn), nIn(nInIn), txdata(nullptr) {}
TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const PrecomputedTransactionData& txdataIn) : txTo(txToIn), nIn(nInIn), txdata(&txdataIn) {}
bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode) const override;
TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(nullptr) {}
TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {}
bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override;
bool CheckLockTime(const CScriptNum& nLockTime) const override;
bool CheckSequence(const CScriptNum& nSequence) const override;
};
Expand All @@ -148,10 +154,10 @@ class MutableTransactionSignatureChecker : public TransactionSignatureChecker
const CTransaction txTo;

public:
MutableTransactionSignatureChecker(const CMutableTransaction* txToIn, unsigned int nInIn) : TransactionSignatureChecker(&txTo, nInIn), txTo(*txToIn) {}
MutableTransactionSignatureChecker(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amount) : TransactionSignatureChecker(&txTo, nInIn, amount), txTo(*txToIn) {}
};

bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* error = nullptr);
bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = nullptr);
bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* error = nullptr);

#endif // BITCOIN_SCRIPT_INTERPRETER_H
4 changes: 2 additions & 2 deletions src/script/ismine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey)

if (keystore.HaveWatchOnly(scriptPubKey)) {
// TODO: This could be optimized some by doing some work after the above solver
CScript scriptSig;
return ProduceSignature(DummySignatureCreator(&keystore), scriptPubKey, scriptSig) ? ISMINE_WATCH_SOLVABLE : ISMINE_WATCH_UNSOLVABLE;
SignatureData sigs;
return ProduceSignature(DummySignatureCreator(&keystore), scriptPubKey, sigs) ? ISMINE_WATCH_SOLVABLE : ISMINE_WATCH_UNSOLVABLE;
}
return ISMINE_NO;
}
2 changes: 1 addition & 1 deletion src/script/sigcache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class CachingTransactionSignatureChecker : public TransactionSignatureChecker
bool store;

public:
CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, PrecomputedTransactionData& txdataIn, bool storeIn=true) : TransactionSignatureChecker(txToIn, nInIn, txdataIn), store(storeIn) {}
CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amount, PrecomputedTransactionData& txdataIn, bool storeIn=true) : TransactionSignatureChecker(txToIn, nInIn, amount, txdataIn), store(storeIn) {}

bool VerifySignature(const std::vector<unsigned char>& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const override;
};
Expand Down
Loading