From 658b7afd1871c10fd7f4667202d33c56e9ccbe27 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 13 Nov 2018 15:46:43 +0300 Subject: [PATCH 1/5] Make error messages re payload a bit more specific --- qa/rpc-tests/dip3-deterministicmns.py | 2 +- src/evo/cbtx.cpp | 4 ++-- src/evo/providertx.cpp | 8 ++++---- src/evo/specialtx.cpp | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/qa/rpc-tests/dip3-deterministicmns.py b/qa/rpc-tests/dip3-deterministicmns.py index e9129374ae..c192343a72 100755 --- a/qa/rpc-tests/dip3-deterministicmns.py +++ b/qa/rpc-tests/dip3-deterministicmns.py @@ -655,7 +655,7 @@ class DIP3Test(BitcoinTestFramework): address = node.getnewaddress() key = node.getnewaddress() blsKey = node.bls('generate') - assert_raises_jsonrpc(None, "bad-tx-type", node.protx, 'register_fund', address, '127.0.0.1:10000', key, blsKey['public'], key, 0, address) + assert_raises_jsonrpc(None, "bad-tx-type-dip3", node.protx, 'register_fund', address, '127.0.0.1:10000', key, blsKey['public'], key, 0, address) def test_success_create_protx(self, node): address = node.getnewaddress() diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 9d0d5cd904..2eef16fa4a 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -20,7 +20,7 @@ bool CheckCbTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidatio CCbTx cbTx; if (!GetTxPayload(tx, cbTx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); + return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-payload"); } if (cbTx.nVersion > CCbTx::CURRENT_VERSION) { @@ -45,7 +45,7 @@ bool CheckCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindex, C CCbTx cbTx; if (!GetTxPayload(*block.vtx[0], cbTx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); + return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-payload"); } if (pindex) { diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index 0db1149e47..5e32efb017 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -80,7 +80,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid CProRegTx ptx; if (!GetTxPayload(tx, ptx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); + return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); } if (ptx.nVersion > CProRegTx::CURRENT_VERSION) { @@ -207,7 +207,7 @@ bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVa CProUpServTx ptx; if (!GetTxPayload(tx, ptx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); + return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); } if (ptx.nVersion > CProRegTx::CURRENT_VERSION) { @@ -259,7 +259,7 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal CProUpRegTx ptx; if (!GetTxPayload(tx, ptx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); + return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); } if (ptx.nVersion > CProRegTx::CURRENT_VERSION) { @@ -335,7 +335,7 @@ bool CheckProUpRevTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal CProUpRevTx ptx; if (!GetTxPayload(tx, ptx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); + return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); } if (ptx.nVersion > CProRegTx::CURRENT_VERSION) { diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index f73e72c616..56e6474a77 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -38,7 +38,7 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali return CheckCbTx(tx, pindexPrev, state); } - return state.DoS(10, false, REJECT_INVALID, "bad-tx-type"); + return state.DoS(10, false, REJECT_INVALID, "bad-tx-type-check"); } bool ProcessSpecialTx(const CTransaction& tx, const CBlockIndex* pindex, CValidationState& state) @@ -57,7 +57,7 @@ bool ProcessSpecialTx(const CTransaction& tx, const CBlockIndex* pindex, CValida return true; // nothing to do } - return state.DoS(100, false, REJECT_INVALID, "bad-tx-type"); + return state.DoS(100, false, REJECT_INVALID, "bad-tx-type-proc"); } bool UndoSpecialTx(const CTransaction& tx, const CBlockIndex* pindex) From 8bd5b231bf2e4dfd65b2828cd061764bfa1ccbe5 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 13 Nov 2018 15:47:28 +0300 Subject: [PATCH 2/5] Log mempool payload errors instead of crashing via assert --- src/txmempool.cpp | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 9021314c47..46b4c4b74c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -444,7 +444,8 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { CProRegTx proTx; if (!GetTxPayload(tx, proTx)) { - assert(false); + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return false; } mapProTxAddresses.emplace(proTx.addr, tx.GetHash()); mapProTxPubKeyIDs.emplace(proTx.keyIDOwner, tx.GetHash()); @@ -455,13 +456,15 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { CProUpServTx proTx; if (!GetTxPayload(tx, proTx)) { - assert(false); + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return false; } mapProTxAddresses.emplace(proTx.addr, tx.GetHash()); } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { CProUpRegTx proTx; if (!GetTxPayload(tx, proTx)) { - assert(false); + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return false; } mapProTxBlsPubKeyHashes.emplace(proTx.pubKeyOperator.GetHash(), tx.GetHash()); } @@ -825,7 +828,8 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx) if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { CProRegTx proTx; if (!GetTxPayload(tx, proTx)) { - assert(false); + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return; } if (mapProTxAddresses.count(proTx.addr)) { @@ -842,7 +846,8 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx) } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { CProUpServTx proTx; if (!GetTxPayload(tx, proTx)) { - assert(false); + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return; } if (mapProTxAddresses.count(proTx.addr)) { @@ -854,7 +859,8 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx) } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { CProUpRegTx proTx; if (!GetTxPayload(tx, proTx)) { - assert(false); + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return; } removeProTxPubKeyConflicts(tx, proTx.pubKeyOperator); @@ -1142,8 +1148,10 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { LOCK(cs); if (tx.nType == TRANSACTION_PROVIDER_REGISTER) { CProRegTx proTx; - if (!GetTxPayload(tx, proTx)) - assert(false); + if (!GetTxPayload(tx, proTx)) { + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return true; // i.e. can't decode payload == conflict + } if (mapProTxAddresses.count(proTx.addr) || mapProTxPubKeyIDs.count(proTx.keyIDOwner) || mapProTxBlsPubKeyHashes.count(proTx.pubKeyOperator.GetHash())) return true; if (!proTx.collateralOutpoint.hash.IsNull() && mapProTxCollaterals.count(proTx.collateralOutpoint)) @@ -1151,14 +1159,18 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { return false; } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) { CProUpServTx proTx; - if (!GetTxPayload(tx, proTx)) - assert(false); + if (!GetTxPayload(tx, proTx)) { + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return true; // i.e. can't decode payload == conflict + } auto it = mapProTxAddresses.find(proTx.addr); return it != mapProTxAddresses.end() && it->second != proTx.proTxHash; } else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { CProUpRegTx proTx; - if (!GetTxPayload(tx, proTx)) - assert(false); + if (!GetTxPayload(tx, proTx)) { + LogPrintf("%s: ERROR: Invalid transaction payload, tx: %s", __func__, tx.ToString()); + return true; // i.e. can't decode payload == conflict + } auto it = mapProTxBlsPubKeyHashes.find(proTx.pubKeyOperator.GetHash()); return it != mapProTxBlsPubKeyHashes.end() && it->second != proTx.proTxHash; } From c975a986b2c6e12439571006093023e14a4f8cce Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 14 Nov 2018 00:17:16 +0300 Subject: [PATCH 3/5] no cs_main in specialtxes --- src/evo/cbtx.cpp | 5 ----- src/evo/providertx.cpp | 8 -------- src/evo/specialtx.cpp | 2 -- 3 files changed, 15 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 2eef16fa4a..711aabb409 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -12,8 +12,6 @@ bool CheckCbTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - AssertLockHeld(cs_main); - if (!tx.IsCoinBase()) { return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-invalid"); } @@ -37,8 +35,6 @@ bool CheckCbTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidatio // This can only be done after the block has been fully processed, as otherwise we won't have the finished MN list bool CheckCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindex, CValidationState& state) { - AssertLockHeld(cs_main); - if (block.vtx[0]->nType != TRANSACTION_COINBASE) { return true; } @@ -63,7 +59,6 @@ bool CheckCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindex, C bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, CValidationState& state) { - AssertLockHeld(cs_main); LOCK(deterministicMNManager->cs); CDeterministicMNList tmpMNList; diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index 5e32efb017..dbae4eb566 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -76,8 +76,6 @@ static bool CheckInputsHash(const CTransaction& tx, const ProTx& proTx, CValidat bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - AssertLockHeld(cs_main); - CProRegTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); @@ -203,8 +201,6 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - AssertLockHeld(cs_main); - CProUpServTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); @@ -255,8 +251,6 @@ bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVa bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - AssertLockHeld(cs_main); - CProUpRegTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); @@ -331,8 +325,6 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal bool CheckProUpRevTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - AssertLockHeld(cs_main); - CProUpRevTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index 56e6474a77..6352b6e660 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -16,8 +16,6 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - AssertLockHeld(cs_main); - if (tx.nVersion < 3 || tx.nType == TRANSACTION_NORMAL) return true; From b843696638f34a11f87404839f2e975c9bd8ba6a Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 14 Nov 2018 11:51:02 +0300 Subject: [PATCH 4/5] Be more specific about tx version in conditions We don't want to set rules for version 4+ txes atm. --- src/evo/deterministicmns.cpp | 2 +- src/evo/specialtx.cpp | 6 +++--- src/primitives/transaction.h | 4 ++-- src/script/interpreter.cpp | 2 +- src/wallet/wallet.cpp | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 97bbb8df4a..724b33cd33 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -676,7 +676,7 @@ int64_t CDeterministicMNManager::GetSpork15Value() bool CDeterministicMNManager::IsProTxWithCollateral(const CTransactionRef& tx, uint32_t n) { - if (tx->nVersion < 3 || tx->nType != TRANSACTION_PROVIDER_REGISTER) { + if (tx->nVersion != 3 || tx->nType != TRANSACTION_PROVIDER_REGISTER) { return false; } CProRegTx proTx; diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index 6352b6e660..cd2a1d0bb7 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -16,7 +16,7 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { - if (tx.nVersion < 3 || tx.nType == TRANSACTION_NORMAL) + if (tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL) return true; if (pindexPrev && VersionBitsState(pindexPrev, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0003, versionbitscache) != THRESHOLD_ACTIVE) { @@ -41,7 +41,7 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali bool ProcessSpecialTx(const CTransaction& tx, const CBlockIndex* pindex, CValidationState& state) { - if (tx.nVersion < 3 || tx.nType == TRANSACTION_NORMAL) { + if (tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL) { return true; } @@ -60,7 +60,7 @@ bool ProcessSpecialTx(const CTransaction& tx, const CBlockIndex* pindex, CValida bool UndoSpecialTx(const CTransaction& tx, const CBlockIndex* pindex) { - if (tx.nVersion < 3 || tx.nType == TRANSACTION_NORMAL) { + if (tx.nVersion != 3 || tx.nType == TRANSACTION_NORMAL) { return true; } diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index cfcd66afe2..c2c2d1d556 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -260,7 +260,7 @@ public: s << vin; s << vout; s << nLockTime; - if (this->nVersion >= 3 && this->nType != TRANSACTION_NORMAL) + if (this->nVersion == 3 && this->nType != TRANSACTION_NORMAL) s << vExtraPayload; } @@ -339,7 +339,7 @@ struct CMutableTransaction READWRITE(vin); READWRITE(vout); READWRITE(nLockTime); - if (this->nVersion >= 3 && this->nType != TRANSACTION_NORMAL) { + if (this->nVersion == 3 && this->nType != TRANSACTION_NORMAL) { READWRITE(vExtraPayload); } } diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index d86696cb8b..bc7e58acdd 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1112,7 +1112,7 @@ public: SerializeOutput(s, nOutput); // Serialize nLockTime ::Serialize(s, txTo.nLockTime); - if (txTo.nVersion >= 3 && txTo.nType != TRANSACTION_NORMAL) + if (txTo.nVersion == 3 && txTo.nType != TRANSACTION_NORMAL) ::Serialize(s, txTo.vExtraPayload); } }; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9e88f53646..3ac1434b6b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2975,7 +2975,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool ov coinControl.Select(txin.prevout); int nExtraPayloadSize = 0; - if (tx.nVersion >= 3 && tx.nType != TRANSACTION_NORMAL) + if (tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL) nExtraPayloadSize = (int)tx.vExtraPayload.size(); CReserveKey reservekey(this); From 6761fa49f1ef0d20576a0f87eb56809eb78cf063 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 14 Nov 2018 16:59:10 +0300 Subject: [PATCH 5/5] More checks for tx type --- src/evo/cbtx.cpp | 4 ++++ src/evo/providertx.cpp | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 711aabb409..b7a8577606 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -12,6 +12,10 @@ bool CheckCbTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { + if (tx.nType != TRANSACTION_COINBASE) { + return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-type"); + } + if (!tx.IsCoinBase()) { return state.DoS(100, false, REJECT_INVALID, "bad-cbtx-invalid"); } diff --git a/src/evo/providertx.cpp b/src/evo/providertx.cpp index dbae4eb566..42967ddba6 100644 --- a/src/evo/providertx.cpp +++ b/src/evo/providertx.cpp @@ -76,6 +76,10 @@ static bool CheckInputsHash(const CTransaction& tx, const ProTx& proTx, CValidat bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { + if (tx.nType != TRANSACTION_PROVIDER_REGISTER) { + return state.DoS(100, false, REJECT_INVALID, "bad-protx-type"); + } + CProRegTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); @@ -201,6 +205,10 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { + if (tx.nType != TRANSACTION_PROVIDER_UPDATE_SERVICE) { + return state.DoS(100, false, REJECT_INVALID, "bad-protx-type"); + } + CProUpServTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); @@ -251,6 +259,10 @@ bool CheckProUpServTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVa bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { + if (tx.nType != TRANSACTION_PROVIDER_UPDATE_REGISTRAR) { + return state.DoS(100, false, REJECT_INVALID, "bad-protx-type"); + } + CProUpRegTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload"); @@ -325,6 +337,10 @@ bool CheckProUpRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVal bool CheckProUpRevTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) { + if (tx.nType != TRANSACTION_PROVIDER_UPDATE_REVOKE) { + return state.DoS(100, false, REJECT_INVALID, "bad-protx-type"); + } + CProUpRevTx ptx; if (!GetTxPayload(tx, ptx)) { return state.DoS(100, false, REJECT_INVALID, "bad-protx-payload");