wallet: fetch pool txs in pruned form

[jeffro256]

This will greatly reduce communication overhead when syncing the mempool. Thanks to @Rucknium for inquiring into the details of the wallet sync protocol, which caused me to notice that pool txs are requested with proofs attached.

[rbrunner7]

Funny, it looks to me as if the capability to get pruned pool transactions from the server was only added quite recently in #8076 by @j-berman, in method core_rpc_server::on_get_blocks, but then actually asking for those in pruned form somehow fell through the cracks.

One question that I could not fully answer by looking at the (quite complex of course) code in wallet2.cpp: Some of the proofs, like spend proofs, seem to do something with signatures, and that's data that's not contained in a pruned tx if I understand correctly. So the question arises: If the wallet learns about a tx first from the pool, in pruned form, and later somebody wants to make a proof, is really all data there to do so, or will the wallet calculate it, or fetch it from the daemon?

Did anybody happen to test that already?

[j-berman]

This function isn't called when syncing, it's called when retrieving a wallet's history. Wallets still make the call to populate the history often though, this PR is an improvement (that I did indeed miss).

It should retrieve txs in pruned form when syncing here:

monero/src/wallet/wallet2.cpp

Lines 3040 to 3048 in cc73fe7

	void wallet2::pull_blocks(bool first, bool try_incremental, uint64_t start_height, uint64_t &blocks_start_height, const std::list<crypto::hash> &short_chain_history, std::vector<cryptonote::block_complete_entry> &blocks, std::vector<cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::block_output_indices> &o_indices, uint64_t &current_height, std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>>& process_pool_txs)
	{
	cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::request req = AUTO_VAL_INIT(req);
	cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::response res = AUTO_VAL_INIT(res);
	req.block_ids = short_chain_history;
	MDEBUG("Pulling blocks: start_height " << start_height);
	req.prune = true;

If the wallet learns about a tx first from the pool, in pruned form, and later somebody wants to make a proof, is really all data there to do so, or will the wallet calculate it, or fetch it from the daemon?

The wallet doesn't need past proofs to make a new proof. Pruned data can be discarded for purposes of tx construction (pruned nodes don't even have large swathes of prunable data stored and you can still sync with a pruned node and construct a tx).

[rbrunner7]

@j-berman : Not sure what you mean, in current master the pruned boolean already gets set in wallet2::pull_blocks, and now @jeffro256 's PR adds it to wallet2::update_pool_state.

But yeah, please forget the question in my comment, you are right, I clearly wasn't thinking far enough: Any tx that a wallet requests from its daemon is potentially pruned, any time, because the daemon's blockchain may be pruned. So it's clear the wallet already knows how to deal with that fact when building any proofs, and fetching pruned transactions from the pool can't possibly make a difference.

[rbrunner7]

Good catch, @Rucknium and @jeffro256 , such a juicy long-hanging fruit :)

[rbrunner7]

Is there still time to PR this to the release branch and get it into the next point update?

[j-berman]

wallet2::update_pool_state (the function updated in this PR) isn't called when syncing; it's called when retrieving a wallet's transfers. wallet2::pull_blocks is called when syncing and does already request txs in pruned from.

[selsta]

@rbrunner7 yes, it will be included in the release

@jeffro256 please open against release branch

[rbrunner7]

Not that it really matters, but I think at least for simplewallet there really is a little twist to the story, according to the comment I left in front of update_pool_state:

// simplewallet does NOT update the pool info during automatic refresh to avoid disturbing interactive
// messages and prompts. When it finally calls this method here "to catch up" so to say we can't use
// incremental update anymore, because with that we might miss some txs altogether.

[rbrunner7]

Er, sorry, my bad again, that's more or less the same statement that you also made, @j-berman :)