[monero-project/monero] Bp multi aggregation pippenger (#4219)

stoffu commented on this pull request.

> difficulty_type cumulative_difficulty = m_blockchain_storage->get_db().get_block_cumulative_difficulty(block_height);
uint64_t coins_generated = m_blockchain_storage->get_db().get_block_already_generated_coins(block_height);

— bp.block_size = block_size;
+ bp.block_size = block_weight;

Why not rename `block_package::block_size` as well?

> @@ -86,7 +86,7 @@ namespace cryptonote {
return CRYPTONOTE_MAX_TX_SIZE;
}
//————————————————————————————————
— bool get_block_reward(size_t median_size, size_t current_block_size, uint64_t already_generated_coins, uint64_t &reward, uint8_t version) {
+ bool get_block_reward(size_t median_size, size_t current_block_weight, uint64_t already_generated_coins, uint64_t &reward, uint8_t version) {

Why not rename `median_size`?

>
— // divide in two steps, since the divisor must be 32 bits, but DYNAMIC_FEE_PER_KB_BASE_BLOCK_REWARD isn’t
— div128_32(hi, lo, DYNAMIC_FEE_PER_KB_BASE_BLOCK_REWARD / 1000000, &hi, &lo);
+ uint64_t unscaled_fee_base = (fee_base * min_block_weight / median_block_weight);
+ lo = mul128(unscaled_fee_base, block_reward, &hi);
+ static_assert(DYNAMIC_FEE_BASE_BLOCK_REWARD % 1000000 == 0, «DYNAMIC_FEE_BASE_BLOCK_REWARD must be divisible by 1000000»);
+ static_assert(DYNAMIC_FEE_BASE_BLOCK_REWARD / 1000000 <= std::numeric_limits::max(), «DYNAMIC_FEE_BASE_BLOCK_REWARD is too large»);
+
+ // divide in two steps, since the divisor must be 32 bits, but DYNAMIC_FEE_BASE_BLOCK_REWARD isn’t
+ div128_32(hi, lo, DYNAMIC_FEE_BASE_BLOCK_REWARD / 1000000, &hi, &lo);

These lines are supposed to stay the same as before, right? Why bother renaming `DYNAMIC_FEE_PER_KB_BASE_BLOCK_REWARD` to `DYNAMIC_FEE_BASE_BLOCK_REWARD`?

> @@ -491,7 +491,7 @@ namespace cryptonote

// the non-simple version is slightly smaller, but assumes all real inputs
// are on the same index, so can only be used if there just one ring.
— bool use_simple_rct = sources.size() > 1 || range_proof_type == rct::RangeProofMultiOutputBulletproof || range_proof_type == rct::RangeProofBulletproof;
+ bool use_simple_rct = sources.size() > 1 || range_proof_type == rct::RangeProofMultiOutputBulletproof || range_proof_type == rct::RangeProofBulletproof || range_proof_type == rct::RangeProofPaddedBulletproof;

Isn’t it cleaner to write the condition as `!(sources.size() == 1 && range_proof_type == rct::RangeProofBorromean)` so that it can work when more types are added to the BP construction?

>
return true;
}
//———————————————————————————
bool tx_memory_pool::add_tx(transaction &tx, tx_verification_context& tvc, bool keeped_by_block, bool relayed, bool do_not_relay, uint8_t version)
{
crypto::hash h = null_hash;
— size_t blob_size = 0;
+ size_t blob_size = 0, tx_weight = 0;

`tx_weight` is unused.

> @@ -558,7 +559,7 @@ bool t_rpc_command_executor::print_blockchain_info(uint64_t start_block_index, u
std::cout << std::endl; std::cout << "height: " << header.height << ", timestamp: " << header.timestamp - << ", size: " << header.block_size << ", transactions: " << header.num_txes << std::endl + << ", size: " << header.block_size << ", weight " << header.block_weight << ", transactions: " << header.num_txes << std::endl Bikeshedding: missing a colon `:` > @@ -5579,6 +5626,7 @@ uint64_t wallet2::get_fee_multiplier(uint32_t priority, int fee_algorithm) const
static const uint64_t old_multipliers[3] = {1, 2, 3};
static const uint64_t new_multipliers[3] = {1, 20, 166};
static const uint64_t newer_multipliers[4] = {1, 4, 20, 166};
+ static const uint64_t newest_multipliers[4] = {1, 5, 25, 1000};

I don’t really like this naming convention. How about putting them into a single vector?
«`c++
static const std::vector> multipliers =
{
{1, 2, 3},
{1, 20, 166},
{1, 4, 20, 166},
{1, 5, 25, 1000},
};
«`

>
— const uint64_t fee_per_kb = get_per_kb_fee();
+ const uint64_t base_fee = get_base_fee();

`fee_per_kb` used below for computing `const uint64_t min_fee` needs updating.

> @@ -8423,16 +8529,15 @@ std::vector wallet2::create_transactions_from(const crypton

// here, check if we need to sent tx and start a new one
LOG_PRINT_L2(«Considering whether to create a tx now, » << tx.selected_transfers.size() << " inputs, tx limit " - << upper_transaction_size_limit); - const size_t estimated_rct_tx_size = estimate_tx_size(use_rct, tx.selected_transfers.size(), fake_outs_count, tx.dsts.size() + 1, extra.size(), bulletproof); - bool try_tx = (unused_dust_indices.empty() && unused_transfers_indices.empty()) || ( estimated_rct_tx_size >= TX_SIZE_TARGET(upper_transaction_size_limit));
+ << upper_transaction_weight_limit); + const size_t estimated_rct_tx_weight = estimate_tx_weight(use_rct, tx.selected_transfers.size(), fake_outs_count, tx.dsts.size() + 2, extra.size(), bulletproof); Why +2 to `tx.dsts.size()`? Isn't +1 enough to account for the dummy output? > {
— if (m_upper_transaction_size_limit > 0)
— return m_upper_transaction_size_limit;
— uint64_t full_reward_zone = use_fork_rules(5, 10) ? CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V5 : use_fork_rules(2, 10) ? CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V2 : CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V1;
+ if (m_upper_transaction_weight_limit > 0)
+ return m_upper_transaction_weight_limit;
+ uint64_t full_reward_zone = use_fork_rules(8, 10) ? (CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V5 / 2) : use_fork_rules(5, 10) ? CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V5 : use_fork_rules(2, 10) ? CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V2 : CRYPTONOTE_BLOCK_GRANTED_FULL_REWARD_ZONE_V1;

Isn’t this way clearer as per tx_pool.cpp?
«`c++
if (use_fork_rules(8, 10))
return full_reward_zone / 2 — CRYPTONOTE_COINBASE_BLOB_RESERVED_SIZE;
else
return full_reward_zone — CRYPTONOTE_COINBASE_BLOB_RESERVED_SIZE;
«`

> @@ -867,7 +867,6 @@ namespace wallet_rpc
bool spent;
uint64_t global_index;
std::string tx_hash;
— uint64_t tx_size;

Bump the RPC version?