Split header MMR (and sync MMR) out from txhashset #3004
Conversation
|
👍 this makes a lot of sense to me, to split it out from txhashset. I will take some tests in next days. btw, May I ask why we constructed a MMR for header? what's the main benefit comparing to original pure database solution? I searched the origin PR #1716 but it didn't mention these. |
|
Oh, Yes! now I remember it:-) thanks for your answer. |
antiochp
force-pushed
the
split_header_mmr
branch
3 times, most recently
from
4436cb6
to
2f380cd
Compare
antiochp
force-pushed
the
split_header_mmr
branch
3 times, most recently
from
b873c4d
to
a06900a
Compare
|
@garyyu Did you get chance to test this? I'd like to merge soon if we can as it simplifies a lot of the rewind logic. |
| return Some(header); | ||
| if let Ok(hash_at_height) = header_pmmr.get_header_hash_by_height(header.height) { | ||
| if let Ok(header_at_height) = self.chain().get_block_header(&hash_at_height) { | ||
| if header.hash() == header_at_height.hash() { |
There was a problem hiding this comment.
This check shouldn't be necessary anymore since we're retrieving by hash.
There was a problem hiding this comment.
Ahh looks like we can simplify this, but we do still need the check.
The core of this is to check the provided header against the header at that height on the current chain - if they have the same hash then the header is on the current chain.
- look up the header based on hash
- look up the header on our chain based on this height
- check these are the same header by hash
But we don't need to actually look the header up based on hash_at_height as we can just compare this hash directly.
if let Ok(header) = self.chain().get_block_header(&hash) {
if let Ok(hash_at_height) = header_pmmr.get_header_hash_by_height(header.height) {
if header.hash() == hash_at_height {
return Some(header);
}
}
}
there is *always* a deadlock in there when we make changes like this...
and pass in header_pmmr for header lookups
antiochp
force-pushed
the
split_header_mmr
branch
from
a06900a
to
c0712a8
Compare
|
Rebased on master to pull in #3015 These changes has made the LOC grow quite a bit in this PR but its mainly just a refactor to deal with the breakout of the header_pmmr from the txhashset. |
|
@antiochp Please let me know if it's ready for another reviewing, thanks. |
|
@garyyu yes please! |
* wip * sync sort of works now * get rid of the deadlock during compaction there is *always* a deadlock in there when we make changes like this... * cleanup how we rebuild the sync MMR on init * cleanup rewind logic * roll the "fix invalid root" changes into this PR * move rebuild_height_pos_index into txhashset and pass in header_pmmr for header lookups * cleanup and remember to setup sync head on init * cleanup unnecessary ref muts * rebuild_height_pos_index when writing txhashset
This PR introduces an explicit separation between the txhashset and the header MMR.
We need both to fully validate full blocks (the output, rangeproofs and kernels are validated w.r.t. the corresponding header, which in turn is validated against the existing header MMR).
But we also need to be able to manipulate the header MMR in isolation - for example when validating "header first" and during initial header sync.
Some thoughts around what we do currently -
This PR moves the header MMR out from under the txhashset and into the local chain instance directly.
We now need a "pair" of extensions when processing full blocks -
Both extensions are rewound independently, but consistently to support various fork/reorg scenarios. We rewind to a single consistent common ancestor (similar to a 3 way merge in git).
i.e. Our header MMR has diverged from the main chain due to a peer advertising previously unseen headers.
Making these changes has allowed a lot of code to be simplified and removed where we no longer need to handle edge cases and complex scenarios across the various MMRs when keeping them all consistent.
TODO -