Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spec compliant merge shares #261
Spec compliant merge shares #261
Changes from 18 commits
494db15
0d15df4
3f0d808
8535122
8bae88b
cbb5d76
b523831
f0f38a6
014a00e
0896a3b
96eafc7
c3d897a
28f9769
9ff16f5
39ce26a
8794411
2b28c59
d118716
3ae1f79
dfe2a07
e9f3a2e
221b0fb
37316e8
e3736d6
632bb37
3ca2afa
0f930fd
91c3989
043812d
76d7a4b
1cb4030
a88db3b
2aad8fd
887aa08
40a57c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic doesn't seem to conform to specs. Potential namespace IDs: https://github.com/lazyledger/lazyledger-specs/blob/de5f4f74f56922e9fa735ef79d9e6e6492a2bad1/specs/consensus.md#reserved-namespace-ids. Namespaces up to and including
NAMESPACE_ID_MAX_RESERVED = 255
are reserved, not messages: https://github.com/lazyledger/lazyledger-specs/blob/de5f4f74f56922e9fa735ef79d9e6e6492a2bad1/specs/consensus.md#constants.There should also be a check somewhere that a reserved namespace ID that's up to 255 but not specifically reserved is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out! added issue #67 in the lazyledger-app to add a check in
MsgWirePayForMessage
'sValidateBasic
method, which get's called with every call toCheckTx
.Added a
case
for this along with a namespace var inconsts.go
to represent the max namespaceID https://github.com/lazyledger/lazyledger-core/blob/d118716769036925491d0412772693a55546ca2c/types/shares.go#L227Should I also add the const
NamespaceIDMaxReserved
and use that to constructMaxReservedNamespaceID
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, the setup you have now should be sufficient I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One additional check that needs to be somewhere (probably in a future PR so as not to overload this one) is that padding shares between a transaction and a message must have namespace ID
TAIL_TRANSACTION_PADDING_NAMESPACE_ID
(all the way at the bottom of https://github.com/lazyledger/lazyledger-specs/blob/de5f4f74f56922e9fa735ef79d9e6e6492a2bad1/specs/data_structures.md#arranging-available-data-into-shares, under the giant figures).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this can serve as a kind of blueprint for the pseudo-code that could end up in the spec.