Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wallet2: validate fetched block height and parent hash #9344

Merged
merged 2 commits into from
Jul 16, 2024
Merged

wallet2: validate fetched block height and parent hash #9344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c634c26
wallet2: use start_height consistently as const in process_parsed_blocks
valldrac Jul 4, 2024
a026d5a
wallet2: validate fetched block height and parent hash
valldrac May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions src/wallet/wallet2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2891,6 +2891,10 @@ void wallet2::process_new_blockchain_entry(const cryptonote::block& b, const cry
"block transactions=" + std::to_string(bche.txs.size()) +
" not match with daemon response size=" + std::to_string(parsed_block.o_indices.indices.size()));

THROW_WALLET_EXCEPTION_IF(height != m_blockchain.size(), error::wallet_internal_error,
"New blockchain entry mismatch: block height " + std::to_string(height) +
" is not the expected next height " + std::to_string(m_blockchain.size()));

//handle transactions from new block

//optimization: seeking only for blocks that are not older then the wallet creation time plus 1 day. 1 day is for possible user incorrect time setup
Expand Down Expand Up @@ -3108,13 +3112,12 @@ void wallet2::pull_hashes(uint64_t start_height, uint64_t &blocks_start_height,
hashes = std::move(res.m_block_ids);
}
//----------------------------------------------------------------------------------------------------
void wallet2::process_parsed_blocks(uint64_t start_height, const std::vector<cryptonote::block_complete_entry> &blocks, const std::vector<parsed_block> &parsed_blocks, uint64_t& blocks_added, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache)
void wallet2::process_parsed_blocks(const uint64_t start_height, const std::vector<cryptonote::block_complete_entry> &blocks, const std::vector<parsed_block> &parsed_blocks, uint64_t& blocks_added, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache)
{
size_t current_index = start_height;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you moved current_index to where it's used the first time, but this is an unrelated change for this PR. As a result, half of the changes in this function are unnecessary.

Patches should be self contained. A good rule of thumb is to have one patch per separate issue, feature, or logical change. Also, no other changes, such as random whitespace changes, reindentation, or fixing typos, spelling, or wording, unless user visible. Following the code style of the particular chunk of code you're modifying is encouraged. Proper squashing should be done (eg, if you're making a buggy patch, then a later patch to fix the bug, both patches should be merged).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change is necessary to prevent future issues. I had to choose between two variables, neither of which was correct for my PR needs. is_in_bounds(start_height) must be called before start_height is used, but it was accessed via current_index, which is mutated later in the function. This mix-up could lead to future issues.

Such a trivial change is easy to review and quick to fix in any merge conflict. Leaving it as is would be incorrect, because the current code already mixes mutable and immutable variables. Probably reviewing the commit history would show that this mix-up comes from previous merges that made minimal changes without considering the context.

I can split the commit in two to clarify the change if necessary, but creating another PR only for this seems excessive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting into two commits is fine. And while you're at it, start_height should then be declared as const uint64_t in function definition and declaration, if it's not supposed to be changed in the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've split into two commits and declared start_height as const.

blocks_added = 0;

THROW_WALLET_EXCEPTION_IF(blocks.size() != parsed_blocks.size(), error::wallet_internal_error, "size mismatch");
THROW_WALLET_EXCEPTION_IF(!m_blockchain.is_in_bounds(current_index), error::out_of_hashchain_bounds_error);
THROW_WALLET_EXCEPTION_IF(!m_blockchain.is_in_bounds(start_height), error::out_of_hashchain_bounds_error);

tools::threadpool& tpool = tools::threadpool::getInstanceForCompute();
tools::threadpool::waiter waiter(tpool);
Expand All @@ -3125,8 +3128,22 @@ void wallet2::process_parsed_blocks(uint64_t start_height, const std::vector<cry
num_txes += 1 + parsed_blocks[i].txes.size();
tx_cache_data.resize(num_txes);
size_t txidx = 0;
crypto::hash prev_block_id;
bool has_prev_block = m_blockchain.is_in_bounds(start_height - 1);
if (has_prev_block) {
prev_block_id = m_blockchain[start_height - 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a check above that start_height is in bounds, but you never check that start_height - 1 is in bounds. It must be checked too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you just shouldn't init prev_block_id if it's not in bounds, instead of throwing an exception like above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. It could fail if the blockchain is trimmed to one block. I just fixed it.

}
for (size_t i = 0; i < blocks.size(); ++i)
{
if (has_prev_block) {
THROW_WALLET_EXCEPTION_IF(prev_block_id != parsed_blocks[i].block.prev_id, error::wallet_internal_error,
"Parent block hash mismatch at height " + std::to_string(start_height + i) +
": expected " + string_tools::pod_to_hex(prev_block_id) +
", but received a new block with prev_id " + string_tools::pod_to_hex(parsed_blocks[i].block.prev_id));
}
prev_block_id = parsed_blocks[i].hash;
has_prev_block = true;

THROW_WALLET_EXCEPTION_IF(parsed_blocks[i].txes.size() != parsed_blocks[i].block.tx_hashes.size(),
error::wallet_internal_error, "Mismatched parsed_blocks[i].txes.size() and parsed_blocks[i].block.tx_hashes.size()");
if (should_skip_block(parsed_blocks[i].block, start_height + i))
Expand Down Expand Up @@ -3266,6 +3283,7 @@ void wallet2::process_parsed_blocks(uint64_t start_height, const std::vector<cry

hwdev.set_mode(hw::device::NONE);

size_t current_index = start_height;
size_t tx_cache_data_offset = 0;
for (size_t i = 0; i < blocks.size(); ++i)
{
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/wallet2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,7 +1698,7 @@ namespace tools
void pull_hashes(uint64_t start_height, uint64_t& blocks_start_height, const std::list<crypto::hash> &short_chain_history, std::vector<crypto::hash> &hashes);
void fast_refresh(uint64_t stop_height, uint64_t &blocks_start_height, std::list<crypto::hash> &short_chain_history, bool force = false);
void pull_and_parse_next_blocks(bool first, bool try_incremental, uint64_t start_height, uint64_t &blocks_start_height, std::list<crypto::hash> &short_chain_history, const std::vector<cryptonote::block_complete_entry> &prev_blocks, const std::vector<parsed_block> &prev_parsed_blocks, std::vector<cryptonote::block_complete_entry> &blocks, std::vector<parsed_block> &parsed_blocks, std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>>& process_pool_txs, bool &last, bool &error, std::exception_ptr &exception);
void process_parsed_blocks(uint64_t start_height, const std::vector<cryptonote::block_complete_entry> &blocks, const std::vector<parsed_block> &parsed_blocks, uint64_t& blocks_added, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL);
void process_parsed_blocks(const uint64_t start_height, const std::vector<cryptonote::block_complete_entry> &blocks, const std::vector<parsed_block> &parsed_blocks, uint64_t& blocks_added, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL);
bool accept_pool_tx_for_processing(const crypto::hash &txid);
void process_unconfirmed_transfer(bool incremental, const crypto::hash &txid, wallet2::unconfirmed_transfer_details &tx_details, bool seen_in_pool, std::chrono::system_clock::time_point now, bool refreshed);
void process_pool_info_extent(const cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::response &res, std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &process_txs, bool refreshed);
Expand Down
Loading