diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 34489699b..b48af7721 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -8912,6 +8912,26 @@ void wallet2::get_outs(std::vector> COMMAND_RPC_GET_OUTPUTS_BIN::request req = AUTO_VAL_INIT(req); COMMAND_RPC_GET_OUTPUTS_BIN::response daemon_resp = AUTO_VAL_INIT(daemon_resp); + // The secret picking order contains outputs in the order that we selected them. + // + // We will later sort the output request entries in a pre-determined order so that the daemon + // that we're requesting information from doesn't learn any information about the true spend + // for each ring. However, internally, we want to prefer to construct our rings using the + // outputs that we picked first versus outputs picked later. + // + // The reason why is because each consecutive output pick within a ring becomes increasing less + // statistically independent from other picks, since we pick outputs from a finite set + // *without replacement*, due to the protocol not allowing duplicate ring members. This effect + // is exacerbated by the fact that we pick 1.5x + 75 as many outputs as we need per RPC + // request to account for unusable outputs. This effect is small, but non-neglibile and gets + // worse with larger ring sizes. + std::vector secret_picking_order; + + // Convenience/safety lambda to make sure that both output lists req.outputs and secret_picking_order are updated together + // Each ring section of req.outputs gets sorted later after selecting all outputs for that ring + const auto add_output_to_lists = [&req, &secret_picking_order](const get_outputs_out &goo) + { req.outputs.push_back(goo); secret_picking_order.push_back(goo); }; + std::unique_ptr gamma; if (has_rct) gamma.reset(new gamma_picker(rct_offsets)); @@ -9046,7 +9066,7 @@ void wallet2::get_outs(std::vector> if (out < num_outs) { MINFO("Using it"); - req.outputs.push_back({amount, out}); + add_output_to_lists({amount, out}); ++num_found; seen_indices.emplace(out); if (out == td.m_global_output_index) @@ -9068,12 +9088,12 @@ void wallet2::get_outs(std::vector> if (num_outs <= requested_outputs_count) { for (uint64_t i = 0; i < num_outs; i++) - req.outputs.push_back({amount, i}); + add_output_to_lists({amount, i}); // duplicate to make up shortfall: this will be caught after the RPC call, // so we can also output the amounts for which we can't reach the required // mixin after checking the actual unlockedness for (uint64_t i = num_outs; i < requested_outputs_count; ++i) - req.outputs.push_back({amount, num_outs - 1}); + add_output_to_lists({amount, num_outs - 1}); } else { @@ -9082,7 +9102,7 @@ void wallet2::get_outs(std::vector> { num_found = 1; seen_indices.emplace(td.m_global_output_index); - req.outputs.push_back({amount, td.m_global_output_index}); + add_output_to_lists({amount, td.m_global_output_index}); LOG_PRINT_L1("Selecting real output: " << td.m_global_output_index << " for " << print_money(amount)); } @@ -9190,7 +9210,7 @@ void wallet2::get_outs(std::vector> seen_indices.emplace(i); picks[type].insert(i); - req.outputs.push_back({amount, i}); + add_output_to_lists({amount, i}); ++num_found; MDEBUG("picked " << i << ", " << num_found << " now picked"); } @@ -9204,7 +9224,7 @@ void wallet2::get_outs(std::vector> // we'll error out later while (num_found < requested_outputs_count) { - req.outputs.push_back({amount, 0}); + add_output_to_lists({amount, 0}); ++num_found; } } @@ -9214,6 +9234,10 @@ void wallet2::get_outs(std::vector> [](const get_outputs_out &a, const get_outputs_out &b) { return a.index < b.index; }); } + THROW_WALLET_EXCEPTION_IF(req.outputs.size() != secret_picking_order.size(), error::wallet_internal_error, + "bug: we did not update req.outputs/secret_picking_order in tandem"); + + // List all requested outputs to debug log if (ELPP->vRegistry()->allowed(el::Level::Debug, MONERO_DEFAULT_LOG_CATEGORY)) { std::map> outs; @@ -9334,18 +9358,21 @@ void wallet2::get_outs(std::vector> } } - // then pick others in random order till we reach the required number - // since we use an equiprobable pick here, we don't upset the triangular distribution - std::vector order; - order.resize(requested_outputs_count); - for (size_t n = 0; n < order.size(); ++n) - order[n] = n; - std::shuffle(order.begin(), order.end(), crypto::random_device{}); - + // While we are still lacking outputs in this result ring, in our secret pick order... LOG_PRINT_L2("Looking for " << (fake_outputs_count+1) << " outputs of size " << print_money(td.is_rct() ? 0 : td.amount())); - for (size_t o = 0; o < requested_outputs_count && outs.back().size() < fake_outputs_count + 1; ++o) + for (size_t ring_pick_idx = base; ring_pick_idx < base + requested_outputs_count && outs.back().size() < fake_outputs_count + 1; ++ring_pick_idx) { - size_t i = base + order[o]; + const get_outputs_out attempted_output = secret_picking_order[ring_pick_idx]; + + // Find the index i of our pick in the request/response arrays + size_t i; + for (i = base; i < base + requested_outputs_count; ++i) + if (req.outputs[i].index == attempted_output.index) + break; + THROW_WALLET_EXCEPTION_IF(i == base + requested_outputs_count, error::wallet_internal_error, + "Could not find index of picked output in requested outputs"); + + // Try adding this output's information to result ring if output isn't invalid LOG_PRINT_L2("Index " << i << "/" << requested_outputs_count << ": idx " << req.outputs[i].index << " (real " << td.m_global_output_index << "), unlocked " << daemon_resp.outs[i].unlocked << ", key " << daemon_resp.outs[i].key); tx_add_fake_output(outs, req.outputs[i].index, daemon_resp.outs[i].key, daemon_resp.outs[i].mask, td.m_global_output_index, daemon_resp.outs[i].unlocked, valid_public_keys_cache); }