-
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
change(rpc): Populate blockcommitmenthash
and defaultroot
fields in the getblocktemplate RPC
#5751
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5751 +/- ##
==========================================
+ Coverage 78.73% 78.76% +0.02%
==========================================
Files 307 307
Lines 38730 38737 +7
==========================================
+ Hits 30496 30512 +16
+ Misses 8234 8225 -9 |
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, I had a quick skim of the code, and it looks good. I'll have a better look at this tomorrow, I was mainly fixing tests today.
Since we're getting the height, hash, and history tree from the database, they could come from different tip blocks. So we'll need to do a consistency check - the easiest way is to check that the tip is the same before and after all the database queries.
If you'd like to get started on the consistency check, we'll need something like this:
zebra/zebra-state/src/service/read/address/balance.rs
Lines 29 to 92 in f573365
/// Returns the total transparent balance for the supplied [`transparent::Address`]es. | |
/// | |
/// If the addresses do not exist in the non-finalized `chain` or finalized `db`, returns zero. | |
pub fn transparent_balance( | |
chain: Option<Arc<Chain>>, | |
db: &ZebraDb, | |
addresses: HashSet<transparent::Address>, | |
) -> Result<Amount<NonNegative>, BoxError> { | |
let mut balance_result = finalized_transparent_balance(db, &addresses); | |
// Retry the finalized balance query if it was interrupted by a finalizing block | |
for _ in 0..FINALIZED_ADDRESS_INDEX_RETRIES { | |
if balance_result.is_ok() { | |
break; | |
} | |
balance_result = finalized_transparent_balance(db, &addresses); | |
} | |
let (mut balance, finalized_tip) = balance_result?; | |
// Apply the non-finalized balance changes | |
if let Some(chain) = chain { | |
let chain_balance_change = | |
chain_transparent_balance_change(chain, &addresses, finalized_tip); | |
balance = apply_balance_change(balance, chain_balance_change).expect( | |
"unexpected amount overflow: value balances are valid, so partial sum should be valid", | |
); | |
} | |
Ok(balance) | |
} | |
/// Returns the total transparent balance for `addresses` in the finalized chain, | |
/// and the finalized tip height the balances were queried at. | |
/// | |
/// If the addresses do not exist in the finalized `db`, returns zero. | |
// | |
// TODO: turn the return type into a struct? | |
fn finalized_transparent_balance( | |
db: &ZebraDb, | |
addresses: &HashSet<transparent::Address>, | |
) -> Result<(Amount<NonNegative>, Option<Height>), BoxError> { | |
// # Correctness | |
// | |
// The StateService can commit additional blocks while we are querying address balances. | |
// Check if the finalized state changed while we were querying it | |
let original_finalized_tip = db.tip(); | |
let finalized_balance = db.partial_finalized_transparent_balance(addresses); | |
let finalized_tip = db.tip(); | |
if original_finalized_tip != finalized_tip { | |
// Correctness: Some balances might be from before the block, and some after | |
return Err("unable to get balance: state was committing a block".into()); | |
} | |
let finalized_tip = finalized_tip.map(|(height, _hash)| height); | |
Ok((finalized_balance, finalized_tip)) | |
} |
Yep, this all looks good, apart from my questions above. When it's ready, I'd also like to see a unit test, or the output of a manual comparison with zcashd. Our zcash-rpc-diff script will do all the necessary checks for you. Can you also tag this PR with a priority and the RPC tag, and set yourself as the owner? |
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, looks good!
I found some state accesses outside the tip check, let me know if you want help moving them.
Synchronizing zcashd and zebrad to do a manual diff test. |
Actually, i think this test can be done in the context of #5686 |
We won't know if this PR works until we test it. We already have a test that will run it and check for errors, and another test that takes a snapshot. But we need to check we've calculated the new fields correctly before it merges. That way, we don't have to fix it later during other tests, like I had to in PR #5761. Since it's a manual test, we need to do it for every significant change. (If it was an automatic test, we could just add it to CI once, and it would catch any mistakes.) |
I will do the test, no problem. just need to have zcashd and zebrad in sync which should happen soon. |
This comment was marked as resolved.
This comment was marked as resolved.
We have several differences, most of them looks expected as we have different transactions which crates differences in the There are a few that might require further analysis in the context of #5686 In this PR we are particularly interested in Test 1:
Test 2:
Test 3:
|
blockcommitmenthash
and defaultroot
missing fieldsblockcommitmenthash
and defaultroot
missing fields
blockcommitmenthash
and defaultroot
missing fieldsblockcommitmenthash
and defaultroot
fields in the getblocktemplate RPC
Here's
I can explain all the differences, and the |
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.
Looks good to me!
We might want to cancel/restart template generation if the chain tip changes before the it's ready instead of the consistency check in relevant_chain_and_history_tree
when adding support for long polling.
Wait it's getting mempool transactions after the ChainInfo
state request.
Motivation
We want to populate the missing fields of the
getblocktemplate
rpc call.Closes #5579 and closes #5455
Depends-On: #5761, #5747
Solution
Get needed data.
Review
I think anyone can review, @teor2345 maybe will want to take a look.
Reviewer Checklist