feat(proof/service)!: check that proof height is less than available threshold

[vgonkivs]

Overview

Fixes #35

TBD:

• Should we make headThreshold configurable because different networks can have different block time?
• Should we blacklist peer in this case?

Checklist

• New and updated code has appropriate documentation
• New and updated code has new and/or updated testing
• Required CI checks are passing
• Visual proof for any user facing features like CLI or documentation updates
• Linked issues closed with keywords

[codecov-commenter]

Codecov Report

Merging #36 (7822259) into main (b00ce70) will decrease coverage by 2.46%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   67.09%   64.63%   -2.46%     
==========================================
  Files           6        6              
  Lines         392      410      +18     
==========================================
+ Hits          263      265       +2     
- Misses         95      109      +14     
- Partials       34       36       +2     

Files	Coverage Δ
fraudserv/service.go	68.34% <20.00%> (-5.70%)	⬇️

[Wondertan]

Should we make headThreshold configurable because different networks can have different block time?

We work with height here, so independently of the block time 20 its high enough value to mitigate false negatives.

Should we blacklist peer in this case?

I don't think so

[renaynay]

Is the plan here to use the syncer head? If so, then I think this should be fine. Should we also sanity-check for heights that are too low (far away from networkHead in the backwards direction) ? Maybe this is a question for john and ismail

[vgonkivs]

I think for now it's not necessary because nodes will anyway store all headers. But this will make sense when we will have pruning. ah I got what you mean. I think it makes sense, just to avoid additional work in verification process. So if we receive unknown fp from the past and we see that network head is more than the fp height, we can skip it(Reject). But this verification should be done after checking it locally.

cc @adlerjohn @liamsi

[vgonkivs]

Yes. We will use syncer Head.

[adlerjohn]

Should we also sanity-check for heights that are too low (far away from networkHead in the backwards direction)

I don't think so, no. Any valid fraud proof in the history of the locally-canonical chain should cause the node to halt.

[nashqueue]

Should we also sanity-check for heights that are too low (far away from networkHead in the backwards direction)

I don't think so, no. Any valid fraud proof in the history of the locally-canonical chain should cause the node to halt.

Could you not induce fraud through a long-range attack, that would halt the node (eclipse it) but is not the canonical chain, because of forward syncing (we don't have backward syncing yet )? This can happen because we handle 2 valid headers as FIFO, not as fraud.

[Wondertan]

This is possible, assuming we don't have backward sync and celestiaorg/go-header#108 is not merged. But we should not introduce complexity here for something that will be obsolete soon after mainnet.

[gupadhyaya]

i am not sure whether this check is really useful as in the current logic right after this check we try to get the extended header for this future FP and ignore validation anyways. This will be useful, if the logic where to store any future FPs (wrt current node's canonical chain or local head). @vgonkivs

[Wondertan]

i am not sure whether this check is really useful as in the current logic right after this check we try to get the extended header for this future FP and ignore validation anyways

The problem here is when we try to get the extended header, it can block and wait until the header is available. And that header can be requested in far future which becomes a DOS vector. This check prevents that

[Wondertan]

This reminds me this, and we may want to keep that logic in some form of HeaderService inside of go-header. Rollkit will also need it. (not soon)

[vgonkivs]

try to get the extended header for this future FP

The thing is that our store will be blocked on this call, waiting for the height. The pubsub has an internal buffered channel(32 msgs iirc) for handling the events, so if we will stuck on this step. It will be pretty easy for an attacker to create a 32 fp from the future so our proof service will be stuck. The main purpose of this fix is to skip FPs that are bigger than the chain head.

[vgonkivs]

This will be useful, if the logic where to store any future FPs (wrt current node's canonical chain or local head)

How can we store future FPs(with height more than the canonical chain has)? In case if local head is less than the chain head, then we will wait until the node syncs to this height to fetch the header.

[adlerjohn]

Could you not induce fraud through a long-range attack

Well first you'd be making a fork, and that would be detectable and rejected by the node before it would accept fraud proofs. Fraud proofs only need to be checked against the local-canonical fork.