From 26072f1393f869b08ba279a3fb5b5e319d90b2ca Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Fri, 24 May 2019 22:41:39 +0000 Subject: [PATCH 1/3] blockchain: forbid v1 coinbase from v12 --- src/cryptonote_config.h | 1 + src/cryptonote_core/blockchain.cpp | 11 +++++++---- src/cryptonote_core/blockchain.h | 3 ++- tests/core_tests/block_validation.cpp | 15 +++++++++++++++ tests/core_tests/block_validation.h | 12 ++++++++++++ tests/core_tests/chaingen_main.cpp | 1 + 6 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/cryptonote_config.h b/src/cryptonote_config.h index b68bb41e1..c1078a70b 100644 --- a/src/cryptonote_config.h +++ b/src/cryptonote_config.h @@ -150,6 +150,7 @@ #define HF_VERSION_SMALLER_BP 10 #define HF_VERSION_LONG_TERM_BLOCK_WEIGHT 10 #define HF_VERSION_MIN_2_OUTPUTS 12 +#define HF_VERSION_MIN_V2_COINBASE_TX 12 #define PER_KB_FEE_QUANTIZATION_DECIMALS 8 diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 0ea81f19a..28a052e47 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1202,11 +1202,12 @@ difficulty_type Blockchain::get_next_difficulty_for_alternative_chain(const std: // one input, of type txin_gen, with height set to the block's height // correct miner tx unlock time // a non-overflowing tx amount (dubious necessity on this check) -bool Blockchain::prevalidate_miner_transaction(const block& b, uint64_t height) +bool Blockchain::prevalidate_miner_transaction(const block& b, uint64_t height, uint8_t hf_version) { LOG_PRINT_L3("Blockchain::" << __func__); CHECK_AND_ASSERT_MES(b.miner_tx.vin.size() == 1, false, "coinbase transaction in the block has no inputs"); CHECK_AND_ASSERT_MES(b.miner_tx.vin[0].type() == typeid(txin_gen), false, "coinbase transaction in the block has the wrong type"); + CHECK_AND_ASSERT_MES(b.miner_tx.version > 1 || hf_version < HF_VERSION_MIN_V2_COINBASE_TX, false, "Invalid coinbase transaction version"); if(boost::get(b.miner_tx.vin[0]).height != height) { MWARNING("The miner transaction in block has invalid height: " << boost::get(b.miner_tx.vin[0]).height << ", expected: " << height); @@ -1713,6 +1714,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id } // this is a cheap test + const uint8_t hf_version = m_hardfork->get_ideal_version(block_height); if (!m_hardfork->check_for_height(b, block_height)) { LOG_PRINT_L1("Block with id: " << id << std::endl << "has old version for height " << block_height); @@ -1770,7 +1772,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id return false; } - if(!prevalidate_miner_transaction(b, bei.height)) + if(!prevalidate_miner_transaction(b, bei.height, hf_version)) { MERROR_VER("Block with id: " << epee::string_tools::pod_to_hex(id) << " (as alternative) has incorrect miner transaction."); bvc.m_verifivation_failed = true; @@ -3615,9 +3617,10 @@ leave: } // this is a cheap test + const uint8_t hf_version = get_current_hard_fork_version(); if (!m_hardfork->check(bl)) { - MERROR_VER("Block with id: " << id << std::endl << "has old version: " << (unsigned)bl.major_version << std::endl << "current: " << (unsigned)m_hardfork->get_current_version()); + MERROR_VER("Block with id: " << id << std::endl << "has old version: " << (unsigned)bl.major_version << std::endl << "current: " << (unsigned)hf_version); bvc.m_verifivation_failed = true; goto leave; } @@ -3722,7 +3725,7 @@ leave: TIME_MEASURE_START(t3); // sanity check basic miner tx properties; - if(!prevalidate_miner_transaction(bl, blockchain_height)) + if(!prevalidate_miner_transaction(bl, blockchain_height, hf_version)) { MERROR_VER("Block with id: " << id << " failed to pass prevalidation"); bvc.m_verifivation_failed = true; diff --git a/src/cryptonote_core/blockchain.h b/src/cryptonote_core/blockchain.h index d95f2dceb..7c2c07ede 100644 --- a/src/cryptonote_core/blockchain.h +++ b/src/cryptonote_core/blockchain.h @@ -1246,10 +1246,11 @@ namespace cryptonote * * @param b the block containing the miner transaction * @param height the height at which the block will be added + * @param hf_version the consensus rules to apply * * @return false if anything is found wrong with the miner transaction, otherwise true */ - bool prevalidate_miner_transaction(const block& b, uint64_t height); + bool prevalidate_miner_transaction(const block& b, uint64_t height, uint8_t hf_version); /** * @brief validates a miner (coinbase) transaction diff --git a/tests/core_tests/block_validation.cpp b/tests/core_tests/block_validation.cpp index 566ec1440..55312bc15 100644 --- a/tests/core_tests/block_validation.cpp +++ b/tests/core_tests/block_validation.cpp @@ -640,3 +640,18 @@ bool gen_block_invalid_binary_format::check_all_blocks_purged(cryptonote::core& return true; } + +bool gen_block_late_v1_coinbase_tx::generate(std::vector& events) const +{ + BLOCK_VALIDATION_INIT_GENERATE(); + + block blk_1; + generator.construct_block_manually(blk_1, blk_0, miner_account, + test_generator::bf_major_ver | test_generator::bf_minor_ver, + HF_VERSION_MIN_V2_COINBASE_TX, HF_VERSION_MIN_V2_COINBASE_TX); + events.push_back(blk_1); + + DO_CALLBACK(events, "check_block_purged"); + + return true; +} diff --git a/tests/core_tests/block_validation.h b/tests/core_tests/block_validation.h index 4a65b029e..2393e1b01 100644 --- a/tests/core_tests/block_validation.h +++ b/tests/core_tests/block_validation.h @@ -206,3 +206,15 @@ struct gen_block_invalid_binary_format : public test_chain_unit_base private: size_t m_corrupt_blocks_begin_idx; }; + +struct gen_block_late_v1_coinbase_tx : public gen_block_verification_base<1> +{ + bool generate(std::vector& events) const; +}; +template<> +struct get_test_options { + const std::pair hard_forks[3] = {std::make_pair(1, 0), std::make_pair(HF_VERSION_MIN_V2_COINBASE_TX, 1), std::make_pair(0, 0)}; + const cryptonote::test_options test_options = { + hard_forks, 0 + }; +}; diff --git a/tests/core_tests/chaingen_main.cpp b/tests/core_tests/chaingen_main.cpp index cb35cfde2..4ee71466e 100644 --- a/tests/core_tests/chaingen_main.cpp +++ b/tests/core_tests/chaingen_main.cpp @@ -133,6 +133,7 @@ int main(int argc, char* argv[]) GENERATE_AND_PLAY(gen_block_has_invalid_tx); GENERATE_AND_PLAY(gen_block_is_too_big); GENERATE_AND_PLAY(gen_block_invalid_binary_format); // Takes up to 3 hours, if CRYPTONOTE_MINED_MONEY_UNLOCK_WINDOW == 500, up to 30 minutes, if CRYPTONOTE_MINED_MONEY_UNLOCK_WINDOW == 10 + GENERATE_AND_PLAY(gen_block_late_v1_coinbase_tx); // Transaction verification tests GENERATE_AND_PLAY(gen_tx_big_version); From 555dc7c39405ba9ab732485bc4214edef00bce77 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sat, 13 Apr 2019 23:37:55 +0000 Subject: [PATCH 2/3] core: from v12, require consistent ring size for mixable txes We're supposed to have a fixed ring size now Already checked by MLSAG verification, but here seems more intuitive --- src/cryptonote_config.h | 1 + src/cryptonote_core/blockchain.cpp | 31 ++++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/cryptonote_config.h b/src/cryptonote_config.h index c1078a70b..834bd5f22 100644 --- a/src/cryptonote_config.h +++ b/src/cryptonote_config.h @@ -151,6 +151,7 @@ #define HF_VERSION_LONG_TERM_BLOCK_WEIGHT 10 #define HF_VERSION_MIN_2_OUTPUTS 12 #define HF_VERSION_MIN_V2_COINBASE_TX 12 +#define HF_VERSION_SAME_MIXIN 12 #define PER_KB_FEE_QUANTIZATION_DECIMALS 8 diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 28a052e47..3c5c928f6 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -2860,7 +2860,8 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc, if (hf_version >= 2) { size_t n_unmixable = 0, n_mixable = 0; - size_t mixin = std::numeric_limits::max(); + size_t min_actual_mixin = std::numeric_limits::max(); + size_t max_actual_mixin = 0; const size_t min_mixin = hf_version >= HF_VERSION_MIN_MIXIN_10 ? 10 : hf_version >= HF_VERSION_MIN_MIXIN_6 ? 6 : hf_version >= HF_VERSION_MIN_MIXIN_4 ? 4 : 2; for (const auto& txin : tx.vin) { @@ -2885,29 +2886,43 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc, else ++n_mixable; } - if (in_to_key.key_offsets.size() - 1 < mixin) - mixin = in_to_key.key_offsets.size() - 1; + size_t ring_mixin = in_to_key.key_offsets.size() - 1; + if (ring_mixin < min_actual_mixin) + min_actual_mixin = ring_mixin; + if (ring_mixin > max_actual_mixin) + max_actual_mixin = ring_mixin; + } + } + MDEBUG("Mixin: " << min_actual_mixin << "-" << max_actual_mixin); + + if (hf_version >= HF_VERSION_SAME_MIXIN) + { + if (min_actual_mixin != max_actual_mixin) + { + MERROR_VER("Tx " << get_transaction_hash(tx) << " has varying ring size (" << (min_actual_mixin + 1) << "-" << (max_actual_mixin + 1) << "), it should be constant"); + tvc.m_low_mixin = true; + return false; } } - if (((hf_version == HF_VERSION_MIN_MIXIN_10 || hf_version == HF_VERSION_MIN_MIXIN_10+1) && mixin != 10) || (hf_version >= HF_VERSION_MIN_MIXIN_10+2 && mixin > 10)) + if (((hf_version == HF_VERSION_MIN_MIXIN_10 || hf_version == HF_VERSION_MIN_MIXIN_10+1) && min_actual_mixin != 10) || (hf_version >= HF_VERSION_MIN_MIXIN_10+2 && min_actual_mixin > 10)) { - MERROR_VER("Tx " << get_transaction_hash(tx) << " has invalid ring size (" << (mixin + 1) << "), it should be 11"); + MERROR_VER("Tx " << get_transaction_hash(tx) << " has invalid ring size (" << (min_actual_mixin + 1) << "), it should be 11"); tvc.m_low_mixin = true; return false; } - if (mixin < min_mixin) + if (min_actual_mixin < min_mixin) { if (n_unmixable == 0) { - MERROR_VER("Tx " << get_transaction_hash(tx) << " has too low ring size (" << (mixin + 1) << "), and no unmixable inputs"); + MERROR_VER("Tx " << get_transaction_hash(tx) << " has too low ring size (" << (min_actual_mixin + 1) << "), and no unmixable inputs"); tvc.m_low_mixin = true; return false; } if (n_mixable > 1) { - MERROR_VER("Tx " << get_transaction_hash(tx) << " has too low ring size (" << (mixin + 1) << "), and more than one mixable input with unmixable inputs"); + MERROR_VER("Tx " << get_transaction_hash(tx) << " has too low ring size (" << (min_actual_mixin + 1) << "), and more than one mixable input with unmixable inputs"); tvc.m_low_mixin = true; return false; } From d22dfb75948e5ad382758c4731c840bd3f067cd0 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sun, 14 Jul 2019 11:51:32 +0000 Subject: [PATCH 3/3] blockchain: reject rct signatures in coinbase txes from v12 --- src/cryptonote_config.h | 1 + src/cryptonote_core/blockchain.cpp | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/src/cryptonote_config.h b/src/cryptonote_config.h index 834bd5f22..bd6755f48 100644 --- a/src/cryptonote_config.h +++ b/src/cryptonote_config.h @@ -152,6 +152,7 @@ #define HF_VERSION_MIN_2_OUTPUTS 12 #define HF_VERSION_MIN_V2_COINBASE_TX 12 #define HF_VERSION_SAME_MIXIN 12 +#define HF_VERSION_REJECT_SIGS_IN_COINBASE 12 #define PER_KB_FEE_QUANTIZATION_DECIMALS 8 diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 3c5c928f6..b5c4abd08 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1208,6 +1208,13 @@ bool Blockchain::prevalidate_miner_transaction(const block& b, uint64_t height, CHECK_AND_ASSERT_MES(b.miner_tx.vin.size() == 1, false, "coinbase transaction in the block has no inputs"); CHECK_AND_ASSERT_MES(b.miner_tx.vin[0].type() == typeid(txin_gen), false, "coinbase transaction in the block has the wrong type"); CHECK_AND_ASSERT_MES(b.miner_tx.version > 1 || hf_version < HF_VERSION_MIN_V2_COINBASE_TX, false, "Invalid coinbase transaction version"); + + // for v2 txes (ringct), we only accept empty rct signatures for miner transactions, + if (hf_version >= HF_VERSION_REJECT_SIGS_IN_COINBASE && b.miner_tx.version >= 2) + { + CHECK_AND_ASSERT_MES(b.miner_tx.rct_signatures.type == rct::RCTTypeNull, false, "RingCT signatures not allowed in coinbase transactions"); + } + if(boost::get(b.miner_tx.vin[0]).height != height) { MWARNING("The miner transaction in block has invalid height: " << boost::get(b.miner_tx.vin[0]).height << ", expected: " << height);