diff --git a/src/cryptonote_core/master_node_voting.cpp b/src/cryptonote_core/master_node_voting.cpp index 76d9919851f..cee43f75771 100755 --- a/src/cryptonote_core/master_node_voting.cpp +++ b/src/cryptonote_core/master_node_voting.cpp @@ -520,20 +520,34 @@ namespace master_nodes result = crypto::check_signature(hash, key, vote.signature); if (result){ - /*MDEBUG("Signature accepted for " << vote.type << " voter " << vote.index_in_group << "/" << key + MTRACE("Signature accepted for " << vote.type << " voter " << vote.index_in_group << "/" << key << (vote.type == quorum_type::obligations ? " voting for worker " + std::to_string( vote.state_change.worker_index) : "") << " at height " << vote.block_height); - */ + if(vote.type == quorum_type::obligations){ - //MDEBUG("Signature accepted for " << quorum.workers[vote.state_change.worker_index]); + MTRACE("Signature accepted for " << quorum.workers[vote.state_change.worker_index]); } } else { vvc.m_signature_not_valid = true; - MDEBUG("Signature not accepted for MN " << quorum.workers[vote.state_change.worker_index]); + + // NOTE: `worker_index` only exists on obligation votes -- it is the inactive member of the + // vote's union for every other type, so reading it here for e.g. a checkpoint vote both is + // UB and yields attacker-controlled bytes taken from the vote's block hash. Checkpoint + // quorums also leave `workers` empty, and this path never bounds-checks the index (only the + // obligations branch above calls bounds_check_worker_index), so an unguarded read here is an + // out-of-bounds access on an empty vector reachable from any peer that sends us a vote. + // + // Reaching this point on an obligations vote does imply the index was bounds-checked, so + // that branch is safe; every other vote type must log via its validator instead. + if (vote.type == quorum_type::obligations) + MDEBUG("Signature not accepted for MN " << quorum.workers[vote.state_change.worker_index]); + else + MDEBUG("Signature not accepted for " << vote.type << " voter " << vote.index_in_group + << "/" << key << " at height " << vote.block_height); } return result; diff --git a/src/cryptonote_core/pos.cpp b/src/cryptonote_core/pos.cpp index 5f9ee02b97d..762efa8b0b8 100755 --- a/src/cryptonote_core/pos.cpp +++ b/src/cryptonote_core/pos.cpp @@ -672,16 +672,21 @@ void POS::handle_message(void *quorumnet_state, POS::message const &msg) auto &value = quorum[msg.quorum_position]; if (value) return; - if (auto const &hash = context.transient.random_value_hashes.wait.data[msg.quorum_position]; hash) + auto const &hash = context.transient.random_value_hashes.wait.data[msg.quorum_position]; + if (!hash) { - auto derived = blake2b_hash(msg.random_value.value.data, sizeof(msg.random_value.value.data)); - if (derived != *hash) - { - MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg) - << ". Rederived random value hash " << derived << " does not match original hash " - << *hash); - return; - } + MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg) + << ". No random value hash was committed for this quorum position"); + return; + } + + auto derived = blake2b_hash(msg.random_value.value.data, sizeof(msg.random_value.value.data)); + if (derived != *hash) + { + MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg) + << ". Rederived random value hash " << derived << " does not match original hash " + << *hash); + return; } value = msg.random_value.value; @@ -1653,9 +1658,16 @@ round_state send_and_wait_for_signed_blocks(round_context &context, master_nodes if (!enforce_validator_participation_and_timeouts(context, stage, node_list, timed_out, all_received)) return goto_preparing_for_next_round(context); - // Select signatures randomly so we don't always just take the first N required signatures. - // Then sort just the first N required signatures, so signatures are added - // to the block in sorted order, but were chosen randomly. + // NOTE: Collect the quorum positions that signed, in ascending order, and take the first + // POS_BLOCK_REQUIRED_SIGNATURES of them. + // + // Any subset of the participating validators is equally valid here: verification only + // requires exactly N signatures, given in ascending order, from validators set in the + // block's participation bitset (see verify_quorum_signatures). Which subset we pick has no + // further effect either -- a validator's participation is scored from POS.validator_bitset, + // not from the signatures that made it into the block, so leaving a validator out does not + // count against it. Quorum positions are themselves re-randomised every block, so taking + // the lowest N does not favour any particular master node over time. std::array indices = {}; size_t indices_count = 0; @@ -1664,10 +1676,7 @@ round_state send_and_wait_for_signed_blocks(round_context &context, master_nodes if (quorum[index]) indices[indices_count++] = index; - // Random select from first 'N' POS_BLOCK_REQUIRED_SIGNATURES from indices_count entries. assert(indices_count >= master_nodes::POS_BLOCK_REQUIRED_SIGNATURES); - std::array selected = {}; - std::sample(indices.begin(), indices.begin() + indices_count, selected.begin(), selected.size(), tools::rng); // Add Signatures cryptonote::block &final_block = context.transient.signed_block.final_block; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 0664dbc6d2b..d24c0b4342f 100755 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -9802,7 +9802,7 @@ void wallet2::light_wallet_get_outs(std::vector(ores.amount_outs[amount_key].amount) + " not found" ); + THROW_WALLET_EXCEPTION_IF(!found_amount , error::wallet_internal_error, "Outputs for amount " + boost::lexical_cast(amount) + " not found" ); LOG_PRINT_L2("Index " << i << "/" << light_wallet_requested_outputs_count << ": idx " << ores.amount_outs[amount_key].outputs[i].global_index << " (real " << td.m_global_output_index << "), unlocked " << "(always in light)" << ", key " << ores.amount_outs[0].outputs[i].public_key);