Throw errors if messages try to use ParitySharesNamespaceID or TailPaddingNamespaceID
#570
Conversation
rootulp
added
the
chore
Codecov Report
@@ Coverage Diff @@
## master #570 +/- ##
==========================================
+ Coverage 28.87% 29.67% +0.80%
==========================================
Files 16 16
Lines 2040 2052 +12
==========================================
+ Hits 589 609 +20
+ Misses 1392 1382 -10
- Partials 59 61 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. |
x/payment/types/payfordata.go
Outdated
|
|
||
| // TODO: why doesn't this function throw an error if NamespaceID is lower | ||
| // than the max reserved namespace ID? | ||
| // https://github.com/celestiaorg/celestia-app/blob/master/x/payment/types/wirepayfordata.go#L108-L111= |
There was a problem hiding this comment.
[question] for reviewers: is there a reason wirepayfordata performs this check but payfordata doesn't need to?
There was a problem hiding this comment.
why doesn't this function throw an error if NamespaceID is lower. than the max reserved namespace ID?
Nice catch! this makes sense to add imo, as this would stop an honest user from making a mistake and is explicit. Feel free to add this to this PR or we can elsewhere.
As a note, we might also want to add checks via a validate basic method on this struct as well (and proceed to actually call that method). The reason being that validator could include an invalid PFD with message that includes a reserve namespace, and we might not catch it.
This makes things more complicated too, as weird things could happen if someone does manage to include a reserved namespace. It might cause bugs for nodes that would rely on block reconstruction (meaning they eventually call DataFromSquare, which I don't think we have yet) where a message with a reserved namespace makes it in the square, but then doens't make it in the block data when call DataFromSquare.
https://github.com/celestiaorg/celestia-core/blob/ff54db5d24f005696c17051d8e737c4285eba24c/types/share_merging.go#L43-L45
where honest nodes will disregard
The original thinking of why it wasn't added is non-intuitive. Until we have immediate execution, checking that the PFD is valid doesn't really do anything. Any validator can include invalid PFD (ie a bad nonce, or an account with zero funds to pay fees) without consequence other than people yelling at them on Twitter. The only way to actually check for these things is by adding block validity rule via process proposal, where we'd have to throw away the entire block should this occur. If we do that, then we also want to add a check to PrepareProposal to ensure that this type of tx doesn't accidently get in the block, which would cause it to be invalid. This might already be occurring implicitly too. If someone tries to add a message to an in use important namespace, such as the TxNamespacedID ([0,0,0,0,0,0,0,1]), then this will cause an share splitting/merging error, which would cause ProcessProposal to reject the block.
We might want to do this and add block validity mechanisms!! More thinking on this is definitely worth while.
There was a problem hiding this comment.
I think you were going to, but just to over-communicate, we should remove this todo before merging
There was a problem hiding this comment.
added #571, but we might want to split that into a few different issues
x/payment/types/payfordata.go
Outdated
|
|
||
| // TODO: why doesn't this function throw an error if NamespaceID is lower | ||
| // than the max reserved namespace ID? | ||
| // https://github.com/celestiaorg/celestia-app/blob/master/x/payment/types/wirepayfordata.go#L108-L111= |
There was a problem hiding this comment.
why doesn't this function throw an error if NamespaceID is lower. than the max reserved namespace ID?
Nice catch! this makes sense to add imo, as this would stop an honest user from making a mistake and is explicit. Feel free to add this to this PR or we can elsewhere.
As a note, we might also want to add checks via a validate basic method on this struct as well (and proceed to actually call that method). The reason being that validator could include an invalid PFD with message that includes a reserve namespace, and we might not catch it.
This makes things more complicated too, as weird things could happen if someone does manage to include a reserved namespace. It might cause bugs for nodes that would rely on block reconstruction (meaning they eventually call DataFromSquare, which I don't think we have yet) where a message with a reserved namespace makes it in the square, but then doens't make it in the block data when call DataFromSquare.
https://github.com/celestiaorg/celestia-core/blob/ff54db5d24f005696c17051d8e737c4285eba24c/types/share_merging.go#L43-L45
where honest nodes will disregard
The original thinking of why it wasn't added is non-intuitive. Until we have immediate execution, checking that the PFD is valid doesn't really do anything. Any validator can include invalid PFD (ie a bad nonce, or an account with zero funds to pay fees) without consequence other than people yelling at them on Twitter. The only way to actually check for these things is by adding block validity rule via process proposal, where we'd have to throw away the entire block should this occur. If we do that, then we also want to add a check to PrepareProposal to ensure that this type of tx doesn't accidently get in the block, which would cause it to be invalid. This might already be occurring implicitly too. If someone tries to add a message to an in use important namespace, such as the TxNamespacedID ([0,0,0,0,0,0,0,1]), then this will cause an share splitting/merging error, which would cause ProcessProposal to reject the block.
We might want to do this and add block validity mechanisms!! More thinking on this is definitely worth while.
x/payment/types/payfordata.go
Outdated
|
|
||
| // TODO: why doesn't this function throw an error if NamespaceID is lower | ||
| // than the max reserved namespace ID? | ||
| // https://github.com/celestiaorg/celestia-app/blob/master/x/payment/types/wirepayfordata.go#L108-L111= |
There was a problem hiding this comment.
I think you were going to, but just to over-communicate, we should remove this todo before merging
Thanks for over-communicating. I was going to add a link to #571 in the |
Description
Resolves #312