From 22db5c54f4dc0c0a05ea7f7f315c93cbe19bb07d Mon Sep 17 00:00:00 2001 From: Azat Nizametdinov Date: Fri, 26 Apr 2019 15:30:01 +0200 Subject: [PATCH 1/6] Move block reward validation to component Signed-off-by: Azat Nizametdinov --- src/Makefile.am | 2 + src/Makefile.test.include | 1 + src/staking/block_reward_validator.cpp | 71 ++++++++ src/staking/block_reward_validator.h | 37 +++++ .../staking/block_reward_validator_tests.cpp | 157 ++++++++++++++++++ 5 files changed, 268 insertions(+) create mode 100644 src/staking/block_reward_validator.cpp create mode 100644 src/staking/block_reward_validator.h create mode 100644 src/test/staking/block_reward_validator_tests.cpp diff --git a/src/Makefile.am b/src/Makefile.am index 2c89abf3ce..11e82e9d1f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -251,6 +251,7 @@ UNITE_CORE_H = \ snapshot/state.h \ staking/active_chain.h \ staking/block_index_map.h \ + staking/block_reward_validator.h \ staking/block_validation_info.h \ staking/block_validator.h \ staking/coin.h \ @@ -398,6 +399,7 @@ libunite_server_a_SOURCES = \ snapshot/state.cpp \ staking/active_chain.cpp \ staking/block_index_map.cpp \ + staking/block_reward_validator.cpp \ staking/block_validation_info.cpp \ staking/block_validator.cpp \ staking/coin.cpp \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index b26329aa63..a1ef38b647 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -133,6 +133,7 @@ UNITE_TESTS =\ test/snapshot/snapshot_index_tests.cpp \ test/snapshot/state_tests.cpp \ test/snapshot/validation_tests.cpp \ + test/staking/block_reward_validator_tests.cpp \ test/staking/coin_tests.cpp \ test/staking/proof_of_stake_tests.cpp \ test/streams_tests.cpp \ diff --git a/src/staking/block_reward_validator.cpp b/src/staking/block_reward_validator.cpp new file mode 100644 index 0000000000..5352989c1e --- /dev/null +++ b/src/staking/block_reward_validator.cpp @@ -0,0 +1,71 @@ +// Copyright (c) 2019 The Unit-e developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/licenses/MIT. + +#include + +#include +#include +#include + +#include + +namespace staking { + +class BlockRewardValidatorImpl : public BlockRewardValidator { + private: + Dependency m_behavior; + + public: + BlockRewardValidatorImpl( + Dependency behavior) + : m_behavior(behavior) {} + + bool CheckBlockRewards(const CTransaction &coinbase_tx, CValidationState &state, const CBlockIndex &index, + CAmount input_amount, CAmount fees) override { + assert(MoneyRange(input_amount)); + assert(MoneyRange(fees)); + + const CBlockIndex &prev_block = *index.pprev; + CAmount total_reward = fees + m_behavior->CalculateBlockReward(prev_block.nHeight); + + std::size_t num_reward_outputs = 1; + + CAmount output_amount = coinbase_tx.GetValueOut(); + if (output_amount - input_amount > total_reward) { + return state.DoS(100, + error("%s: coinbase pays too much (total output=%d total input=%d expected reward=%d )", + __func__, FormatMoney(output_amount), FormatMoney(input_amount), + FormatMoney(total_reward)), + REJECT_INVALID, "bad-cb-amount"); + } + + // TODO UNIT-E: make the check stricter: if (coinbase_tx.GetValueOut() - input_amount < block_reward) + if (output_amount < input_amount) { + return state.DoS(100, + error("%s: coinbase pays too little (total output=%d total input=%d expected reward=%d )", + __func__, FormatMoney(output_amount), FormatMoney(input_amount), + FormatMoney(total_reward)), + REJECT_INVALID, "bad-cb-spends-too-little"); + } + + CAmount non_reward_out = 0; + for (std::size_t i = num_reward_outputs; i < coinbase_tx.vout.size(); ++i) { + non_reward_out += coinbase_tx.vout[i].nValue; + } + if (non_reward_out > input_amount) { + return state.DoS(100, + error("%s: coinbase spends too much (non-reward output=%d total input=%d)", __func__, + FormatMoney(non_reward_out), FormatMoney(input_amount)), + REJECT_INVALID, "bad-cb-spends-too-much"); + } + + return true; + } +}; + +std::unique_ptr BlockRewardValidator::New( + Dependency behavior) { + return MakeUnique(behavior); +} +} // namespace staking \ No newline at end of file diff --git a/src/staking/block_reward_validator.h b/src/staking/block_reward_validator.h new file mode 100644 index 0000000000..3b114a6ebd --- /dev/null +++ b/src/staking/block_reward_validator.h @@ -0,0 +1,37 @@ +// Copyright (c) 2019 The Unit-e developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/licenses/MIT. + +#ifndef UNIT_E_STAKING_BLOCK_REWARD_VALIDATOR_H +#define UNIT_E_STAKING_BLOCK_REWARD_VALIDATOR_H + +#include +#include +#include + +#include + +class CBlockIndex; +class CTransaction; +class CValidationState; + +namespace staking { + +class BlockRewardValidator { + public: + virtual bool CheckBlockRewards( + const CTransaction &coinbase_tx, + CValidationState &state, + const CBlockIndex &index, + CAmount input_amount, + CAmount fees) = 0; + + virtual ~BlockRewardValidator() = default; + + static std::unique_ptr New( + Dependency); +}; + +} // namespace staking + +#endif //UNIT_E_STAKING_BLOCK_REWARD_VALIDATOR_H diff --git a/src/test/staking/block_reward_validator_tests.cpp b/src/test/staking/block_reward_validator_tests.cpp new file mode 100644 index 0000000000..b71abfb2e8 --- /dev/null +++ b/src/test/staking/block_reward_validator_tests.cpp @@ -0,0 +1,157 @@ +// Copyright (c) 2019 The Unit-e developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://opensource.org/licenses/MIT. + +#include +#include +#include + +#include +#include + +namespace { + +struct Fixture { + + const CAmount total_reward = 10 * UNIT; + + blockchain::Parameters parameters = [this]() { + blockchain::Parameters p = blockchain::Parameters::TestNet(); + p.reward = total_reward; + return p; + }(); + + std::unique_ptr b = + blockchain::Behavior::NewFromParameters(parameters); + + CBlockIndex prev_block = [this]() { + CBlockIndex b; + b.nHeight = 100; + return b; + }(); + + const uint256 block_hash; + CBlockIndex block = [this]() { + CBlockIndex b; + b.pprev = &prev_block; + b.nHeight = prev_block.nHeight + 1; + b.phashBlock = &block_hash; + return b; + }(); + + CTransaction MakeCoinbaseTx(const std::vector &outputs) { + const CTxIn meta_input; + const CTxIn staking_input; + + CMutableTransaction tx; + tx.SetType(TxType::COINBASE); + tx.vin = {meta_input, staking_input}; + for (const auto out : outputs) { + tx.vout.emplace_back(out, CScript()); + } + return tx; + } + + CTransaction MakeCoinbaseTx( + CAmount block_reward, + const std::vector> &finalization_rewards, + const std::vector &outputs) { + const CTxIn meta_input; + const CTxIn staking_input; + + CMutableTransaction tx; + tx.SetType(TxType::COINBASE); + tx.vin = {meta_input, staking_input}; + tx.vout.emplace_back(block_reward, CScript()); + for (const auto &r : finalization_rewards) { + tx.vout.emplace_back(r.second, r.first); + } + for (const auto out : outputs) { + tx.vout.emplace_back(out, CScript()); + } + return tx; + } + + std::unique_ptr GetBlockRewardValidator() { + return staking::BlockRewardValidator::New(b.get()); + } +}; + +} // namespace + +BOOST_AUTO_TEST_SUITE(block_reward_validator_tests) + +BOOST_AUTO_TEST_CASE(valid_reward) { + Fixture f; + const auto validator = f.GetBlockRewardValidator(); + + const CAmount input_amount = 10 * UNIT; + const CAmount fees = 1 * UNIT; + + auto test_valid_outputs = [&](const std::vector outputs) { + CTransaction tx = f.MakeCoinbaseTx(outputs); + CValidationState validation_state; + + const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); + BOOST_CHECK(result); + BOOST_CHECK(validation_state.IsValid()); + }; + test_valid_outputs({1 * UNIT + fees, input_amount}); + test_valid_outputs({1 * UNIT + fees, input_amount / 2, input_amount / 2}); + test_valid_outputs({1 * UNIT + fees + input_amount}); + test_valid_outputs({input_amount}); +} + +BOOST_AUTO_TEST_CASE(total_output_is_too_large) { + Fixture f; + const auto validator = f.GetBlockRewardValidator(); + + const CAmount input_amount = 11 * UNIT; + const CAmount fees = 1 * UNIT; + auto test_invalid_outputs = [&](const std::vector outputs) { + CTransaction tx = f.MakeCoinbaseTx(outputs); + CValidationState validation_state; + + const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); + BOOST_CHECK(!result); + BOOST_CHECK(!validation_state.IsValid()); + BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); + BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-amount"); + }; + test_invalid_outputs({1 * UNIT + fees + 1, input_amount}); + test_invalid_outputs({1 * UNIT + fees, input_amount + 1}); +} + +BOOST_AUTO_TEST_CASE(total_output_is_too_small) { + Fixture f; + const auto validator = f.GetBlockRewardValidator(); + + const CAmount input_amount = 11 * UNIT; + const CAmount fees = 1 * UNIT; + CTransaction tx = f.MakeCoinbaseTx({0, input_amount - 1}); + CValidationState validation_state; + + const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); + BOOST_CHECK(!result); + BOOST_CHECK(!validation_state.IsValid()); + BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); + BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-spends-too-little"); +} + +BOOST_AUTO_TEST_CASE(non_reward_output_is_too_large) { + Fixture f; + const auto validator = f.GetBlockRewardValidator(); + + const CAmount input_amount = 15 * UNIT; + const CAmount fees = 1 * UNIT; + CTransaction tx = f.MakeCoinbaseTx({1 * UNIT, input_amount + fees}); + CValidationState validation_state; + + const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); + BOOST_CHECK(!result); + BOOST_CHECK(!validation_state.IsValid()); + BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); + BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-spends-too-much"); +} + +BOOST_AUTO_TEST_SUITE_END() From 689480c78bb81bb59d513bd0e856550684b9f760 Mon Sep 17 00:00:00 2001 From: Azat Nizametdinov Date: Fri, 10 May 2019 11:34:08 +0200 Subject: [PATCH 2/6] Add invocation of BlockRewardValidator Signed-off-by: Azat Nizametdinov --- src/consensus/tx_verify.cpp | 30 ++---------- src/injector.h | 4 ++ src/test/tx_verify_tests.cpp | 91 ------------------------------------ src/validation.cpp | 10 ++-- 4 files changed, 11 insertions(+), 124 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index dc1ab9173b..3137f33ae0 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -259,33 +259,9 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c } const CAmount value_out = tx.GetValueOut(); - if (tx.IsCoinBase()) { - // The coinbase transaction should spend exactly its inputs and the reward. - // The reward output is by definition in the zeroth output. The reward - // consists of newly minted money (the block reward) and the fees accumulated - // from the transactions. - if (tx.vout.empty()) { - return state.DoS(100, false, REJECT_INVALID, "bad-cb-no-reward", false, - strprintf("coinbase without a reward txout")); - } - const CTxOut &reward_out = tx.vout[0]; - const CAmount reward = reward_out.nValue; - if (nValueIn + reward < value_out) { - return state.DoS(100, false, REJECT_INVALID, "bad-cb-spends-too-much", false, - strprintf("value in (%s) + reward(%s) != value out (%s) in coinbase", - FormatMoney(nValueIn), - FormatMoney(reward), - FormatMoney(value_out))); - } - if (nValueIn + reward > value_out) { - return state.DoS(100, false, REJECT_INVALID, "bad-cb-spends-too-little", false, - strprintf("value in (%s) + reward(%s) != value out (%s) in coinbase", - FormatMoney(nValueIn), - FormatMoney(reward), - FormatMoney(value_out))); - } - } else if (!tx.IsCoinBase()) { - // All other transactions have to spend no more then their inputs. If they spend + // Coinbase outputs are validated in BlockRewardValidator::CheckBlockRewards + if (!tx.IsCoinBase()) { + // All non-coinbase transactions have to spend no more than their inputs. If they spend // less, the change is counted towards the fees which are included in the reward // of the coinbase transaction. if (nValueIn < value_out) { diff --git a/src/injector.h b/src/injector.h index 82f93c9f2f..c63fef9722 100644 --- a/src/injector.h +++ b/src/injector.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -72,6 +73,9 @@ class UnitEInjector : public Injector { staking::BlockValidator, staking::StakeValidator) + COMPONENT(BlockRewardValidator, staking::BlockRewardValidator, staking::BlockRewardValidator::New, + blockchain::Behavior) + COMPONENT(BlockDB, BlockDB, BlockDB::New) COMPONENT(FinalizationParams, finalization::Params, finalization::Params::New, diff --git a/src/test/tx_verify_tests.cpp b/src/test/tx_verify_tests.cpp index 2ec1119a51..a6903ccc5f 100644 --- a/src/test/tx_verify_tests.cpp +++ b/src/test/tx_verify_tests.cpp @@ -31,33 +31,6 @@ BOOST_AUTO_TEST_CASE(check_tx_inputs_no_haz_coins) { BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-txns-inputs-missingorspent"); } -BOOST_AUTO_TEST_CASE(check_tx_inputs_no_reward) { - - const CTxIn meta_input; - BOOST_REQUIRE(meta_input.prevout.IsNull()); - const uint256 some_txid = uint256S("4623a9438473459c466ea4fe87b5a614362e08c47454cf59646e49c5759cb60d"); - const CTxIn staking_input(some_txid, 0); - - CTransaction tx = [&] { - CMutableTransaction tx; - tx.SetType(TxType::COINBASE); - tx.vin = {meta_input, staking_input}; - return tx; - }(); - - mocks::CoinsViewMock utxos; - utxos.mock_AccessCoin.SetResult(Coin(CTxOut(21, CScript()), 1, TxType::REGULAR)); - - CValidationState validation_state; - CAmount fees; - - const bool result = Consensus::CheckTxInputs(tx, validation_state, utxos, 2, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-no-reward"); -} - BOOST_AUTO_TEST_CASE(check_tx_inputs_does_not_access_coinbase_meta_input) { const CTxIn meta_input; @@ -91,68 +64,4 @@ BOOST_AUTO_TEST_CASE(check_tx_inputs_does_not_access_coinbase_meta_input) { BOOST_CHECK(std::find(coins_accessed.begin(), coins_accessed.end(), staking_input.prevout) != coins_accessed.end()); } -BOOST_AUTO_TEST_CASE(check_tx_inputs_rejects_coinbase_that_spends_too_little) { - - const CTxIn meta_input; - BOOST_REQUIRE(meta_input.prevout.IsNull()); - const uint256 some_txid = uint256S("4623a9438473459c466ea4fe87b5a614362e08c47454cf59646e49c5759cb60d"); - const CTxIn staking_input(some_txid, 0); - - const CAmount reward = 21; - const CAmount stake_in = 19; - const CAmount stake_out = stake_in - 1; - - CTransaction tx = [&] { - CMutableTransaction tx; - tx.SetType(TxType::COINBASE); - tx.vin = {meta_input, staking_input}; - tx.vout = {CTxOut(reward, CScript()), CTxOut(stake_out, CScript())}; - return tx; - }(); - - mocks::CoinsViewMock utxos; - utxos.mock_AccessCoin.SetResult(Coin(CTxOut(stake_in, CScript()), 1, TxType::REGULAR)); - - CValidationState validation_state; - CAmount fees; - - const bool result = Consensus::CheckTxInputs(tx, validation_state, utxos, 2, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-spends-too-little"); -} - -BOOST_AUTO_TEST_CASE(check_tx_inputs_rejects_coinbase_that_spends_too_much) { - - const CTxIn meta_input; - BOOST_REQUIRE(meta_input.prevout.IsNull()); - const uint256 some_txid = uint256S("4623a9438473459c466ea4fe87b5a614362e08c47454cf59646e49c5759cb60d"); - const CTxIn staking_input(some_txid, 0); - - const CAmount reward = 21; - const CAmount stake_in = 19; - const CAmount stake_out = stake_in + 1; - - CTransaction tx = [&] { - CMutableTransaction tx; - tx.SetType(TxType::COINBASE); - tx.vin = {meta_input, staking_input}; - tx.vout = {CTxOut(reward, CScript()), CTxOut(stake_out, CScript())}; - return tx; - }(); - - mocks::CoinsViewMock utxos; - utxos.mock_AccessCoin.SetResult(Coin(CTxOut(stake_in, CScript()), 1, TxType::REGULAR)); - - CValidationState validation_state; - CAmount fees; - - const bool result = Consensus::CheckTxInputs(tx, validation_state, utxos, 2, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-spends-too-much"); -} - BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 56dbedde0f..592e3d4e5b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2195,12 +2195,10 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl LogPrint(BCLog::BENCH, " - Connect %u transactions: %.2fms (%.3fms/tx, %.3fms/txin) [%.2fs (%.2fms/blk)]\n", (unsigned)block.vtx.size(), MILLI * (nTime3 - nTime2), MILLI * (nTime3 - nTime2) / block.vtx.size(), nInputs <= 1 ? 0 : MILLI * (nTime3 - nTime2) / (nInputs-1), nTimeConnect * MICRO, nTimeConnect * MILLI / nBlocksTotal); if (!isGenesisBlock) { - CAmount blockReward = nFees + GetComponent()->CalculateBlockReward(pindex->nHeight); - if (block.vtx[0]->GetValueOut() - coinbase_in > blockReward) - return state.DoS(100, - error("ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)", - block.vtx[0]->GetValueOut(), blockReward), - REJECT_INVALID, "bad-cb-amount"); + if (!GetComponent()->CheckBlockRewards(*block.vtx[0], state, *pindex, + coinbase_in, nFees)) { + return false; + } } if (!control.Wait()) From ec6063deda27a136e98a3c89e9eac8831eca6b95 Mon Sep 17 00:00:00 2001 From: Azat Nizametdinov Date: Tue, 30 Apr 2019 17:13:05 +0200 Subject: [PATCH 3/6] Make reward amount check stricter Signed-off-by: Azat Nizametdinov --- src/staking/block_reward_validator.cpp | 8 ++-- .../staking/block_reward_validator_tests.cpp | 45 ++++++------------- 2 files changed, 17 insertions(+), 36 deletions(-) diff --git a/src/staking/block_reward_validator.cpp b/src/staking/block_reward_validator.cpp index 5352989c1e..63ea66c500 100644 --- a/src/staking/block_reward_validator.cpp +++ b/src/staking/block_reward_validator.cpp @@ -27,7 +27,7 @@ class BlockRewardValidatorImpl : public BlockRewardValidator { assert(MoneyRange(fees)); const CBlockIndex &prev_block = *index.pprev; - CAmount total_reward = fees + m_behavior->CalculateBlockReward(prev_block.nHeight); + CAmount total_reward = fees + m_behavior->CalculateBlockReward(index.nHeight); std::size_t num_reward_outputs = 1; @@ -40,8 +40,8 @@ class BlockRewardValidatorImpl : public BlockRewardValidator { REJECT_INVALID, "bad-cb-amount"); } - // TODO UNIT-E: make the check stricter: if (coinbase_tx.GetValueOut() - input_amount < block_reward) - if (output_amount < input_amount) { + // TODO UNIT-E: make the check stricter: if (output_amount - input_amount < total_reward) + if (output_amount - input_amount < total_reward - fees) { return state.DoS(100, error("%s: coinbase pays too little (total output=%d total input=%d expected reward=%d )", __func__, FormatMoney(output_amount), FormatMoney(input_amount), @@ -68,4 +68,4 @@ std::unique_ptr BlockRewardValidator::New( Dependency behavior) { return MakeUnique(behavior); } -} // namespace staking \ No newline at end of file +} // namespace staking diff --git a/src/test/staking/block_reward_validator_tests.cpp b/src/test/staking/block_reward_validator_tests.cpp index b71abfb2e8..1c44d26933 100644 --- a/src/test/staking/block_reward_validator_tests.cpp +++ b/src/test/staking/block_reward_validator_tests.cpp @@ -14,6 +14,7 @@ namespace { struct Fixture { const CAmount total_reward = 10 * UNIT; + const CAmount immediate_reward = 1 * UNIT; blockchain::Parameters parameters = [this]() { blockchain::Parameters p = blockchain::Parameters::TestNet(); @@ -52,26 +53,6 @@ struct Fixture { return tx; } - CTransaction MakeCoinbaseTx( - CAmount block_reward, - const std::vector> &finalization_rewards, - const std::vector &outputs) { - const CTxIn meta_input; - const CTxIn staking_input; - - CMutableTransaction tx; - tx.SetType(TxType::COINBASE); - tx.vin = {meta_input, staking_input}; - tx.vout.emplace_back(block_reward, CScript()); - for (const auto &r : finalization_rewards) { - tx.vout.emplace_back(r.second, r.first); - } - for (const auto out : outputs) { - tx.vout.emplace_back(out, CScript()); - } - return tx; - } - std::unique_ptr GetBlockRewardValidator() { return staking::BlockRewardValidator::New(b.get()); } @@ -86,7 +67,7 @@ BOOST_AUTO_TEST_CASE(valid_reward) { const auto validator = f.GetBlockRewardValidator(); const CAmount input_amount = 10 * UNIT; - const CAmount fees = 1 * UNIT; + const CAmount fees = UNIT / 2; auto test_valid_outputs = [&](const std::vector outputs) { CTransaction tx = f.MakeCoinbaseTx(outputs); @@ -96,10 +77,10 @@ BOOST_AUTO_TEST_CASE(valid_reward) { BOOST_CHECK(result); BOOST_CHECK(validation_state.IsValid()); }; - test_valid_outputs({1 * UNIT + fees, input_amount}); - test_valid_outputs({1 * UNIT + fees, input_amount / 2, input_amount / 2}); - test_valid_outputs({1 * UNIT + fees + input_amount}); - test_valid_outputs({input_amount}); + test_valid_outputs({f.immediate_reward + fees, input_amount}); + test_valid_outputs({f.immediate_reward + fees, input_amount / 2, input_amount / 2}); + test_valid_outputs({f.immediate_reward + fees + input_amount}); + test_valid_outputs({f.immediate_reward + input_amount}); } BOOST_AUTO_TEST_CASE(total_output_is_too_large) { @@ -107,7 +88,7 @@ BOOST_AUTO_TEST_CASE(total_output_is_too_large) { const auto validator = f.GetBlockRewardValidator(); const CAmount input_amount = 11 * UNIT; - const CAmount fees = 1 * UNIT; + const CAmount fees = UNIT / 2; auto test_invalid_outputs = [&](const std::vector outputs) { CTransaction tx = f.MakeCoinbaseTx(outputs); CValidationState validation_state; @@ -118,8 +99,8 @@ BOOST_AUTO_TEST_CASE(total_output_is_too_large) { BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-amount"); }; - test_invalid_outputs({1 * UNIT + fees + 1, input_amount}); - test_invalid_outputs({1 * UNIT + fees, input_amount + 1}); + test_invalid_outputs({f.immediate_reward + fees + 1, input_amount}); + test_invalid_outputs({f.immediate_reward + fees, input_amount + 1}); } BOOST_AUTO_TEST_CASE(total_output_is_too_small) { @@ -127,8 +108,8 @@ BOOST_AUTO_TEST_CASE(total_output_is_too_small) { const auto validator = f.GetBlockRewardValidator(); const CAmount input_amount = 11 * UNIT; - const CAmount fees = 1 * UNIT; - CTransaction tx = f.MakeCoinbaseTx({0, input_amount - 1}); + const CAmount fees = UNIT / 2; + CTransaction tx = f.MakeCoinbaseTx({0, input_amount}); CValidationState validation_state; const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); @@ -143,8 +124,8 @@ BOOST_AUTO_TEST_CASE(non_reward_output_is_too_large) { const auto validator = f.GetBlockRewardValidator(); const CAmount input_amount = 15 * UNIT; - const CAmount fees = 1 * UNIT; - CTransaction tx = f.MakeCoinbaseTx({1 * UNIT, input_amount + fees}); + const CAmount fees = UNIT / 2; + CTransaction tx = f.MakeCoinbaseTx({f.immediate_reward, input_amount + fees}); CValidationState validation_state; const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); From 7c80f0285b0dc60e81361b62ed5c74dc78e52dfa Mon Sep 17 00:00:00 2001 From: Azat Nizametdinov Date: Fri, 10 May 2019 17:38:06 +0200 Subject: [PATCH 4/6] Remove unused variable Signed-off-by: Azat Nizametdinov --- src/staking/block_reward_validator.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/staking/block_reward_validator.cpp b/src/staking/block_reward_validator.cpp index 63ea66c500..6c41c863b2 100644 --- a/src/staking/block_reward_validator.cpp +++ b/src/staking/block_reward_validator.cpp @@ -26,7 +26,6 @@ class BlockRewardValidatorImpl : public BlockRewardValidator { assert(MoneyRange(input_amount)); assert(MoneyRange(fees)); - const CBlockIndex &prev_block = *index.pprev; CAmount total_reward = fees + m_behavior->CalculateBlockReward(index.nHeight); std::size_t num_reward_outputs = 1; From 5c5e03e1ae55a306c8543badc4d246b4222c6635 Mon Sep 17 00:00:00 2001 From: Azat Nizametdinov Date: Mon, 13 May 2019 16:52:07 +0200 Subject: [PATCH 5/6] Validate number of outputs in coinbase transaction Signed-off-by: Azat Nizametdinov --- src/staking/block_reward_validator.cpp | 6 ++++++ .../staking/block_reward_validator_tests.cpp | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/staking/block_reward_validator.cpp b/src/staking/block_reward_validator.cpp index 6c41c863b2..972e627161 100644 --- a/src/staking/block_reward_validator.cpp +++ b/src/staking/block_reward_validator.cpp @@ -29,6 +29,12 @@ class BlockRewardValidatorImpl : public BlockRewardValidator { CAmount total_reward = fees + m_behavior->CalculateBlockReward(index.nHeight); std::size_t num_reward_outputs = 1; + if (coinbase_tx.vout.size() < num_reward_outputs) { + return state.DoS(100, + error("%s: too few coinbase outputs expected at least %d actual %d", __func__, + num_reward_outputs, coinbase_tx.vout.size()), + REJECT_INVALID, "bad-cb-too-few-outputs"); + } CAmount output_amount = coinbase_tx.GetValueOut(); if (output_amount - input_amount > total_reward) { diff --git a/src/test/staking/block_reward_validator_tests.cpp b/src/test/staking/block_reward_validator_tests.cpp index 1c44d26933..6718a28461 100644 --- a/src/test/staking/block_reward_validator_tests.cpp +++ b/src/test/staking/block_reward_validator_tests.cpp @@ -103,6 +103,22 @@ BOOST_AUTO_TEST_CASE(total_output_is_too_large) { test_invalid_outputs({f.immediate_reward + fees, input_amount + 1}); } +BOOST_AUTO_TEST_CASE(no_outputs) { + Fixture f; + const auto validator = f.GetBlockRewardValidator(); + + const CAmount input_amount = 11 * UNIT; + const CAmount fees = UNIT / 2; + CTransaction tx = f.MakeCoinbaseTx({}); + CValidationState validation_state; + + const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); + BOOST_CHECK(!result); + BOOST_CHECK(!validation_state.IsValid()); + BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); + BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-too-few-outputs"); +} + BOOST_AUTO_TEST_CASE(total_output_is_too_small) { Fixture f; const auto validator = f.GetBlockRewardValidator(); From 1a7c4f855a063c3be3cccc13d6c00b243e3b40ad Mon Sep 17 00:00:00 2001 From: Azat Nizametdinov Date: Tue, 14 May 2019 14:08:47 +0200 Subject: [PATCH 6/6] Refactor tests Signed-off-by: Azat Nizametdinov --- src/staking/block_reward_validator.cpp | 2 +- src/staking/block_reward_validator.h | 2 +- .../staking/block_reward_validator_tests.cpp | 59 +++++++++---------- 3 files changed, 29 insertions(+), 34 deletions(-) diff --git a/src/staking/block_reward_validator.cpp b/src/staking/block_reward_validator.cpp index 972e627161..6f4e123127 100644 --- a/src/staking/block_reward_validator.cpp +++ b/src/staking/block_reward_validator.cpp @@ -22,7 +22,7 @@ class BlockRewardValidatorImpl : public BlockRewardValidator { : m_behavior(behavior) {} bool CheckBlockRewards(const CTransaction &coinbase_tx, CValidationState &state, const CBlockIndex &index, - CAmount input_amount, CAmount fees) override { + CAmount input_amount, CAmount fees) const override { assert(MoneyRange(input_amount)); assert(MoneyRange(fees)); diff --git a/src/staking/block_reward_validator.h b/src/staking/block_reward_validator.h index 3b114a6ebd..9db43770e8 100644 --- a/src/staking/block_reward_validator.h +++ b/src/staking/block_reward_validator.h @@ -24,7 +24,7 @@ class BlockRewardValidator { CValidationState &state, const CBlockIndex &index, CAmount input_amount, - CAmount fees) = 0; + CAmount fees) const = 0; virtual ~BlockRewardValidator() = default; diff --git a/src/test/staking/block_reward_validator_tests.cpp b/src/test/staking/block_reward_validator_tests.cpp index 6718a28461..30eeca1de2 100644 --- a/src/test/staking/block_reward_validator_tests.cpp +++ b/src/test/staking/block_reward_validator_tests.cpp @@ -58,6 +58,21 @@ struct Fixture { } }; +void CheckTransactionIsRejected( + const CTransaction &tx, + const std::string &rejection_reason, + const staking::BlockRewardValidator &validator, + const CBlockIndex &block, + const CAmount input_amount, + const CAmount fees) { + CValidationState validation_state; + const bool result = validator.CheckBlockRewards(tx, validation_state, block, input_amount, fees); + BOOST_CHECK(!result); + BOOST_CHECK(!validation_state.IsValid()); + BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); + BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), rejection_reason); +} + } // namespace BOOST_AUTO_TEST_SUITE(block_reward_validator_tests) @@ -89,18 +104,13 @@ BOOST_AUTO_TEST_CASE(total_output_is_too_large) { const CAmount input_amount = 11 * UNIT; const CAmount fees = UNIT / 2; - auto test_invalid_outputs = [&](const std::vector outputs) { - CTransaction tx = f.MakeCoinbaseTx(outputs); - CValidationState validation_state; - const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-amount"); - }; - test_invalid_outputs({f.immediate_reward + fees + 1, input_amount}); - test_invalid_outputs({f.immediate_reward + fees, input_amount + 1}); + CheckTransactionIsRejected( + f.MakeCoinbaseTx({f.immediate_reward + fees + 1, input_amount}), + "bad-cb-amount", *validator, f.block, input_amount, fees); + CheckTransactionIsRejected( + f.MakeCoinbaseTx({f.immediate_reward + fees, input_amount + 1}), + "bad-cb-amount", *validator, f.block, input_amount, fees); } BOOST_AUTO_TEST_CASE(no_outputs) { @@ -109,14 +119,9 @@ BOOST_AUTO_TEST_CASE(no_outputs) { const CAmount input_amount = 11 * UNIT; const CAmount fees = UNIT / 2; - CTransaction tx = f.MakeCoinbaseTx({}); - CValidationState validation_state; - const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-too-few-outputs"); + CTransaction tx = f.MakeCoinbaseTx({}); + CheckTransactionIsRejected(tx, "bad-cb-too-few-outputs", *validator, f.block, input_amount, fees); } BOOST_AUTO_TEST_CASE(total_output_is_too_small) { @@ -125,14 +130,9 @@ BOOST_AUTO_TEST_CASE(total_output_is_too_small) { const CAmount input_amount = 11 * UNIT; const CAmount fees = UNIT / 2; - CTransaction tx = f.MakeCoinbaseTx({0, input_amount}); - CValidationState validation_state; - const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-spends-too-little"); + CTransaction tx = f.MakeCoinbaseTx({0, input_amount}); + CheckTransactionIsRejected(tx, "bad-cb-spends-too-little", *validator, f.block, input_amount, fees); } BOOST_AUTO_TEST_CASE(non_reward_output_is_too_large) { @@ -141,14 +141,9 @@ BOOST_AUTO_TEST_CASE(non_reward_output_is_too_large) { const CAmount input_amount = 15 * UNIT; const CAmount fees = UNIT / 2; - CTransaction tx = f.MakeCoinbaseTx({f.immediate_reward, input_amount + fees}); - CValidationState validation_state; - const bool result = validator->CheckBlockRewards(tx, validation_state, f.block, input_amount, fees); - BOOST_CHECK(!result); - BOOST_CHECK(!validation_state.IsValid()); - BOOST_CHECK_EQUAL(validation_state.GetRejectCode(), REJECT_INVALID); - BOOST_CHECK_EQUAL(validation_state.GetRejectReason(), "bad-cb-spends-too-much"); + CTransaction tx = f.MakeCoinbaseTx({f.immediate_reward, input_amount + fees}); + CheckTransactionIsRejected(tx, "bad-cb-spends-too-much", *validator, f.block, input_amount, fees); } BOOST_AUTO_TEST_SUITE_END()