-
Notifications
You must be signed in to change notification settings - Fork 992
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
Invalid Root #3016
Comments
Looks like the fix of #3005 flipped the coin to another side, now the "Invalid Root" problem could happen in a high possibility once the node see a fork. (both 329991 and 330552 are the fork block). I checked some of my servers, most of those servers upgraded on latest master branch has this |
Interesting. @garyyu, @j01tz can you try with the branch here #3004 and see if you can reproduce the "Invalid Root" problem?
|
I'm spending some time today thinking through possible permutations of block and header processing. I suspect we do not have good test coverage for some of these. |
Can you post your failing test here? |
OK, Here: `Invalid Root` failing test code#[test]
fn process_a_simple_fork() {
let chain_dir = ".grin.simplefork";
clean_output_dir(chain_dir);
global::set_mining_mode(ChainTypes::AutomatedTesting);
let kc = ExtKeychain::from_random_seed(false).unwrap();
let genesis = pow::mine_genesis_block().unwrap();
let last_status = RwLock::new(None);
let adapter = Arc::new(StatusAdapter::new(last_status));
let chain = setup_with_status_adapter(chain_dir, genesis.clone(), adapter.clone());
let prev = chain.head_header().unwrap();
assert_eq!(prev, genesis.header);
// add 1 header + 1 block
let b0 = prepare_block(&kc, &prev, &chain, 1);
chain
.process_block_header(&b0.header, chain::Options::SKIP_POW)
.unwrap();
chain
.process_block(b0.clone(), chain::Options::SKIP_POW)
.unwrap();
let prev = chain.head_header().unwrap();
assert_eq!(prev, b0.header);
// add 1 header + 1 block
let b1 = prepare_block(&kc, &prev, &chain, 1_000_086_937_600_726u64);
chain
.process_block_header(&b1.header, chain::Options::SKIP_POW)
.unwrap();
chain
.process_block(b1.clone(), chain::Options::SKIP_POW)
.unwrap();
// Now mine a fork block and process this block.
let b2 = prepare_block(&kc, &prev, &chain, 1_000_086_937_600_726u64);
chain
.process_block(b2.clone(), chain::Options::SKIP_POW)
.unwrap();
// Check head and header_head both reflect b1 (first come wins).
let head = chain.head().unwrap();
assert_eq!(head, Tip::from_header(&b1.header));
let header_head = chain.header_head().unwrap();
assert_eq!(header_head, Tip::from_header(&b1.header));
// Now mine a block b3 to continue b2.
let prev = b2.header;
let b3 = prepare_block(&kc, &prev, &chain, 3);
chain
.process_block_header(&b3.header, chain::Options::SKIP_POW)
.unwrap();
// now header_head should references b3.
let head_header = chain.header_head().unwrap();
assert_eq!(head_header, Tip::from_header(&b3.header));
// Now process the full block for b3.
chain
.process_block(b3.clone(), chain::Options::SKIP_POW)
.unwrap();
// Check head and header_head both reflect b3, as this is the winning full block at this height.
let head = chain.head().unwrap();
assert_eq!(head, Tip::from_header(&b3.header));
let header_head = chain.header_head().unwrap();
assert_eq!(header_head, Tip::from_header(&b3.header));
clean_output_dir(chain_dir);
} To run this test: $ RUST_BACKTRACE=1 cargo test -p grin_chain -- process_a_simple_fork --nocapture |
Cool thanks. Confirmed this fails on master but passes successfully on the branch from #3004. |
What's the root cause of this |
Still trying to work this out - and yes agreed it would be good to fix this on master. |
Yes, that is almost exactly what happened - the fix in #3005 now allows certain scenarios to correctly handle the header (by correctly rewinding the header MMR) but then when subsequently processing the full block our txhashset MMRs are not correctly rewound. The fix in #3017 ensures the following rules are followed -
Normally these are consistent and track the same chain head. But during periods when these diverge it is critical these rules are followed correctly. |
Should #3017 be able to unstick the node in this state or do I need to remove
|
@j01tz I wish we had some way of allowing a node from recovering from this (mainly for testing purposes for scenarios like this). But right now unfortunately I don't think there is any way to recover. Edit: Tracking some thoughts around recovering from this on a local node here #3018. It should be possible to do this robustly without too much work (and will make our lives so much easier). |
The invalid root issue addressed in #3005 doesn't seem to be solved completely. My node has encountered the case twice today on the latest master branch. Haven't had a chance yet to investigate much further.
The text was updated successfully, but these errors were encountered: