-
Notifications
You must be signed in to change notification settings - Fork 94
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.
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
[r2r] fixed block_header_utxo_loop #1506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix! Please fix these two notes
pub struct BlockHeaderInMemoryStorage { | ||
pub ticker: String, | ||
pub conn: Arc<Mutex<Connection>>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified significantly since BlockHeaderInMemoryStorage
is used in tests only:
pub struct BlockHeaderInMemoryStorage {
pub ticker: String,
// The block headers should be ordered to be able to return the last block header.
pub block_headers: BTreeMap<u64, BlockHeader>,
// This can be used on [`BlockHeaderStorageOps::get_block_height_by_hash`].
pub block_headers_by_hash: HashMap<H256, BlockHeader>,
}
Please also note that get_last_block_height
should be configurable from a test. For example, at the beginning of a test you can just upload one block header with a specified height. Also there could be a field like BlockHeaderInMemoryStorage::last_block_height
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted for next PR. THANKS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more changes please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review iteration!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Only one minor comment. I still don't get why we got No headers available
error with the old code, but this PR is an improvement for sure :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thank you for this fix!
One minor change please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the fix :)
4212b20
@cipig could you please test this fix? There shouldn't be |
Looks good, the error message is gone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approve
fixes this comment: #1482 (comment)