feat: add robustness to monero block extra field handling #5826
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghpbot-tari-project
added
P-acks_required
hansieodendaal
force-pushed
the
ho_extra_field
branch
from
9cc3464
to
d5853a6
Compare
SWvheerden
reviewed
Oct 3, 2023
SWvheerden
reviewed
Oct 3, 2023
hansieodendaal
force-pushed
the
ho_extra_field
branch
2 times, most recently
from
53cd81d
to
50674d9
Compare
SWvheerden
reviewed
Oct 11, 2023
hansieodendaal
force-pushed
the
ho_extra_field
branch
from
50674d9
to
74431a8
Compare
Added robustness to handling the extra field in a Monero block. Use cases for the extra field are: the extra field is changed to add a merge mining hash sub field, the merge mining hash is extracted from the extra field and a Monero block header is verified to contain only one merge mining hash. Parsing an extra field from bytes will always return a valid extra field, even if it does not represent the original extra field. As per Monero consensus rules, a parsing error here will not represent a failure to de-serialize a block, so no need to handle the error in such cases. Having more than one merge mining tag sub field in a block is currently not allowed in the Tari code base, so this is now checked prior to adding the merge mining tag sub field. When adding the merge mining tag sub field, care is taken to not invalidate an existing extra field. If `SubField::Padding(n)` with `n < 255` is the last sub field in the extra field, then appending a new field will always fail deserialization (`ExtraField::try_parse`) - the new field cannot be parsed in that sequence. To circumvent this, we create a new extra field by appending the original extra field sub fields to the merge mining sub field instead.
hansieodendaal
force-pushed
the
ho_extra_field
branch
from
74431a8
to
df1a4ad
Compare
stringhandler
approved these changes
Nov 6, 2023
ghpbot-tari-project
removed
the
P-reviews_required
stringhandler
merged commit 597b9ef
into
tari-project:development
14 of 15 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Added robustness to editing and parsing the extra field in a Monero block. Use cases for the extra field are:
As per Monero consensus rules, a parsing error with
ExtraField::try_parse(raw_extra_field)will not represent a failure to de-serialize a block, as, upon error, it will just return the subfields that could be read. This means that when a Monero block template is requested by the merge mining proxy it can contain an "invalid" extra field, mainly due to the presence of a variable lengthSubField::Padding(n)withn < 255if not the last subfield in the extra field.If we append the merge mining tag subfield to a valid existing extra field with a variable
SubField::Padding(n)withn < 255as the last subfield, the new extra field cannot be parsed successfully. To circumvent this, we insert the merge mining subfield in front of the subfield array instead of appending it at the rear.In the event that we have an invalid extra field to start with, we can now parse the extra field and clean it up in the same process before inserting the merge mining subfield. When extracting the merge mining hash we now also ignore an invalid extra field and return the merge mining hash if present.
Having more than one merge mining tag subfield in a block is currently not allowed in the Tari code base, so this is now checked prior to adding the merge mining tag subfield.
Motivation and Context
See above
How Has This Been Tested?
test_duplicate_append_mm_tagto verify that appending more than one merge mining tag will be blocked prior to validating the Monero block header and that header validation will still fail if an additional merge mining tag is present.fn test_extra_field_with_parsing_error()What process can a PR reviewer use to test or verify this change?
Code walkthrough
Familiarize the monero.rs code base
Breaking Changes