chore: Improvements to Block Footer and Proof Support #1891
Conversation
|
Leaving in draft until @AlfredoG87 's PR #1891 goes in. |
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Nana-EC
force-pushed
the
1880-bump-0-68-1-cn-proto
branch
from
c4b9e3e to
e8360d1
Compare
Nana-EC
changed the title
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (38.88%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #1891 +/- ##
============================================
- Coverage 79.48% 78.91% -0.58%
- Complexity 1193 1200 +7
============================================
Files 128 130 +2
Lines 5684 5781 +97
Branches 610 618 +8
============================================
+ Hits 4518 4562 +44
- Misses 887 934 +47
- Partials 279 285 +6
🚀 New features to boost your workflow:
|
jsync-swirlds
left a comment
jsync-swirlds
left a comment
There was a problem hiding this comment.
A few concerns we might need to address.
| switch (blockProof.proof().kind()) { | ||
| case BLOCK_STATE_PROOF -> this.blockStateProof = blockProof.blockStateProof(); | ||
| case SIGNED_BLOCK_PROOF -> this.tssSignedBlockProof = blockProof.signedBlockProof(); | ||
| case SIGNED_RECORD_FILE_PROOF -> | ||
| this.signedRecordFileProof = blockProof.signedRecordFileProof(); | ||
| default -> { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| blockProofsReceived++; |
There was a problem hiding this comment.
This appears to be assuming only one of any given type of proof. That is not what we expect; there may well be multiples of the same type. The original (add all of them to a set and process all as a set) seems more correct.
There was a problem hiding this comment.
Indeed that was the assumption, good note, will re-approach this
|
|
||
| blockProofsReceived++; | ||
| } | ||
| case REDACTED_ITEM -> LOGGER.log(WARNING, "Redacted item observed in block {0}", blockNumber); |
There was a problem hiding this comment.
This is tricky.
A live block cannot have redaction.
An historical block may.
In the former case the block must be rejected (bad block).
In the latter case, we don't need to log it, we need to include that redacted hash in the correct subtree and proceed.
There was a problem hiding this comment.
Yes tricky indeed, hmm, VerificationSession would have to be aware of the source of the Block.
If it's from a CN publisher then reject otherwise include hash as is without rehashing.
The warning log is to highlight we have things to do in the code.
Might be worth dropping this part and just capturing a ticket to come back to
There was a problem hiding this comment.
Probably better to have an issue filed to revisit.
Generally, because WARNING logs are interpreted as software bugs by some testing processes, INFO would be the highest priority for a "we need to come back and improve this" log.
| blockProofsReceived++; | ||
| } | ||
| case REDACTED_ITEM -> LOGGER.log(WARNING, "Redacted item observed in block {0}", blockNumber); | ||
| case FILTERED_ITEM_HASH -> LOGGER.log(WARNING, "Filtered item hash observed in block {0}", blockNumber); |
There was a problem hiding this comment.
This one is fun.
- From live, this is strictly prohibited
- From storage or another block node in tier 1 this is strictly prohibited.
- From any source in tier 2 this is permitted.
In all cases filtered items still need to be added to the proper subtree as a pre-computed hash (which I do not believe the current hasher supports, future work to handle that).
This suggests that we probably need two different verification plugins. One that is "strict" and runs in tier 1, and another that is "lenient" and may run in tier 2 if an operator chooses. (yes, we could do this with configuration, but I believe we'd be better off and much safer to just have two different plugins).
| case FILTERED_ITEM_HASH -> LOGGER.log(WARNING, "Filtered item hash observed in block {0}", blockNumber); | ||
| case RECORD_FILE -> LOGGER.log(WARNING, "Record file observed in block {0}", blockNumber); | ||
| case UNSET -> LOGGER.log(WARNING, "Unset block item observed in block {1}", kind, blockNumber); | ||
| default -> LOGGER.log(WARNING, "Unknown block item type {0} in block {1}", kind, blockNumber); |
There was a problem hiding this comment.
We shouldn't have default; the enumerated value is comprehensive and we should ensure every known enum is handled (which the compiler will only do if there is no default).
Any enumerated type we don't yet handle should be in a different place (the unknown values map); not mixed in with existing block items.
| case REDACTED_ITEM -> LOGGER.log(WARNING, "Redacted item observed in block {0}", blockNumber); | ||
| case FILTERED_ITEM_HASH -> LOGGER.log(WARNING, "Filtered item hash observed in block {0}", blockNumber); | ||
| case RECORD_FILE -> LOGGER.log(WARNING, "Record file observed in block {0}", blockNumber); | ||
| case UNSET -> LOGGER.log(WARNING, "Unset block item observed in block {1}", kind, blockNumber); |
There was a problem hiding this comment.
This shouldn't happen.
It also probably shouldn't be a warning log; I would expect an info log and immediate failure of the verification (because there is a completely unprocessable block item).
There was a problem hiding this comment.
I believe a new blockitem type we are unaware of would show up as UNSET.
Need to confirm with tests
| } | ||
| case REDACTED_ITEM -> LOGGER.log(WARNING, "Redacted item observed in block {0}", blockNumber); | ||
| case FILTERED_ITEM_HASH -> LOGGER.log(WARNING, "Filtered item hash observed in block {0}", blockNumber); | ||
| case RECORD_FILE -> LOGGER.log(WARNING, "Record file observed in block {0}", blockNumber); |
There was a problem hiding this comment.
We actually need to parse and process these; for now we probably want an info log; not warning.
| private TssSignedBlockProof tssSignedBlockProof; | ||
| private StateProof blockStateProof; | ||
| private SignedRecordFileProof signedRecordFileProof; | ||
|
|
||
| private List<BlockProof> blockProofs = new ArrayList<>(); | ||
| private int blockProofsReceived = 0; |
There was a problem hiding this comment.
This probably won't work. We can have multiple of the same type of block proof.
StateProof type block proofs, in particular, are very likely to have multiple on a given block.
TSS signed proofs are currently expected to be singular, but that may not always be true. Signed record file proof type will probably always be singular, but we perhaps shouldn't assume that (for symmetry, if nothing else).
Reviewer Notes
Related Issue(s)
Use keywords
Fix, Fixes, Fixed, Close, Closes, Closed, Resolve, Resolves, Resolvedto connect an issue to be closed by this pull request.