chainstore: Fix raw blocks getting scanned for links during snapshots #10684
Conversation
We have to save raw blocks to the snapshot, but we should not be scanning them for additional links as if they were CBOR blocks. This cleans the logic a bit (we were checking that the parent was a CBOR block before queueing up the children, but then scanning the children... it was weird). Additionally, more verbose logging is added for the next time ScanForLinks fails (currently very little info was given). Our ScanForLinks callback should only enqueue CBOR for further processing.
|
@ognots do you have a way to test this on the problematic epoch? (2764810) |
yeah, I can find an older snapshot to restore from, and I'll test it. |
| // We exported the ipld block. If it wasn't a CBOR block, there's nothing | ||
| // else to do and we can bail out early as it won't have any links | ||
| // etc. | ||
| if t.c.Prefix().Codec != cid.DagCBOR || t.c.Prefix().MhType == mh.IDENTITY { |
There was a problem hiding this comment.
While it likely doesn't matter (at the moment, we should scan for links in "identity" blocks. In theory, I can inline dag-cbor into an identity hash.
There was a problem hiding this comment.
@Stebalien I never knew the reasons to skip identity hashes, it was something inherited from @frrist code. I can't say if there's an instance of an identity hash in the whole chain history (? genesis or something?)
There was a problem hiding this comment.
Hm. So, we shouldn't include blocks for identity hashes, but we should technically be traversing them and including their children, if any.
On the other hand, I can guarantee that this won't make a difference in practice (at the moment, at least). So we should be good to merge this as-os.
There was a problem hiding this comment.
ok, happy to send follow-up in a few days.
There was a problem hiding this comment.
fwiw I inherited it from:
Line 668 in e945c0d
| // We exported the ipld block. If it wasn't a CBOR block, there's nothing | ||
| // else to do and we can bail out early as it won't have any links | ||
| // etc. | ||
| if t.c.Prefix().Codec != cid.DagCBOR || t.c.Prefix().MhType == mh.IDENTITY { |
There was a problem hiding this comment.
Hm. So, we shouldn't include blocks for identity hashes, but we should technically be traversing them and including their children, if any.
On the other hand, I can guarantee that this won't make a difference in practice (at the moment, at least). So we should be good to merge this as-os.
…filecoin-project#10684) We have to save raw blocks to the snapshot, but we should not be scanning them for additional links as if they were CBOR blocks. This cleans the logic a bit (we were checking that the parent was a CBOR block before queueing up the children, but then scanning the children... it was weird). Additionally, more verbose logging is added for the next time ScanForLinks fails (currently very little info was given). Our ScanForLinks callback should only enqueue CBOR for further processing.
We have to save raw blocks to the snapshot, but we should not be scanning them for additional links as if they were CBOR blocks.
This cleans the logic a bit (we were checking that the parent was a CBOR block before queueing up the children, but then scanning the children... it was weird).
Additionally, more verbose logging is added for the next time ScanForLinks fails (currently very little info was given).
Our ScanForLinks callback should only enqueue CBOR for further processing.
Related Issues
#10672
Proposed Changes
Fixes raw blocks being scanned for links as if they were CBOR.
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps