Feature: added extra data length checks #4805

OlegJakushkin · 2022-10-24T11:27:59Z

Resolves potential issue of Extra Data being too long (more than 32 bytes)

Changes:

Added checks and InvalidConfigurationException thrown at the user in case of wrong values set

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update
Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)
Build related changes
Other (please describe):

Testing

Cosmetic feature. Has its unit tests implemented
Requires testing

Yes
No

In case you checked yes, did you write tests??

Yes
No

Added checks and tests for the ExtraData not to be longer than 32 bytes

an empty line removed

MarekM25 · 2022-10-24T11:37:13Z

src/Nethermind/Nethermind.Consensus/MiningConfig.cs

+                byte[] bytes = Encoding.UTF8.GetBytes(value);
+                if (bytes != null && bytes.Length > 32)
+                {
+                    throw new InvalidConfigurationException($"Extra Data length was more than 32 bytes. You provided: {_extraDataString}",


Did you check if this exception will stop the node?

Yes:
With --Mining.ExtraData=Nethermind-Nethermind-Nethermind-Nethermind
in Debug

in Release

Would prefer a nicer message, not an exception trace.
It is now thrown very early in the pipeline and not handled correctly or it is because it is being packed by other exceptions. I also think exit code might not be used. Can we make this simple error log in next release?

Exception on this line: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Runner/Program.cs#L158

Handling:
https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Runner/Program.cs#L211-L223

MarekM25

Did you consider adding trimming in BlockProducer code?

OlegJakushkin · 2022-10-24T11:59:47Z

@MarekM25 BlockProducers code calls MiningConfig.GetExtraDataBytes() getter directly

* Added 32 bytes check on extra data Added checks and tests for the ExtraData not to be longer than 32 bytes

* Implement TraceStore plugin * Add compression to db * Allow unsafe to avoid allocation * Fix EndBlockTrace to be after Applying rewards * Add ByteArrayPoolList * Make ResettableList implement IReadOnlyCollection * Make DbOnTheRocks default to nameless configuration if named not found * Make BlockTracerBase based on ResettableList * Fixes and improvements * Add plugin to pipelines * fix test * Add temporary traces * Revert "Add temporary traces" This reverts commit 9190ed0. * Add exception data to json rpc errors * Increase MaxDepth to 128 * Refactor product versioning and revise Docker files (#4781) * Upgrade version to 14.4 (#4796) Co-authored-by: Kamil Chodoła <kamil@nethermind.io> * Chiado TTD (#4801) * Updating Fast Sync config files (#4803) Co-authored-by: LukaszRozmej <LukaszRozmej@users.noreply.github.com> * Feature: added extra data length checks (#4805) * Added 32 bytes check on extra data Added checks and tests for the ExtraData not to be longer than 32 bytes * Fix workflow to pass parameters to scripts (#4799) * Autorecover when disconnected chain is found (#4816) * Autorecover when disconnected chain is found * Fix tests * Updating Fast Sync config files (#4819) Co-authored-by: MarekM25 <MarekM25@users.noreply.github.com> * Fix environment variable definition (#4825) * Revert "Fix sepolia invalid block (#4694)" (#4838) This reverts commit 8f28e4a. * Bump build version to 14.5 (#4839) Co-authored-by: Kamil Chodoła <kamil@nethermind.io> * fix transition creation * Don't log error, when initialization is stopped (like when syncing previously downloaded blocks from db) * Add VerifySerialized option * Add serialization tests * Move VerifySerialized to TraceSerializer, add MaxDepth config * fix whitespace * fix null handling in TxBroadcaster * Refactor ITraceSerializer * fix test * undo fix * whitespace fix * test fix * fixes * Set chiado and xdai `Merge.SecondsPerSlot=5` (#4880) * Set chiado `Merge.SecondsPerSlot=5` * Update xdai configs with `Merge.SecondsPerSlot=5` This commit is not relevant before the merge. * Updating Fast Sync config files (#4897) Co-authored-by: LukaszRozmej <LukaszRozmej@users.noreply.github.com> * Set C# version to 10.0 (#4881) * Add gnosischain TTD value (#4901) * Add gnosischain TTD value Scheduled for `Dec 5 2022` assuming 5s block times: `8626000000000000000000058750000000000000000000` * Add EnginePort xdai configs * Add xdai TTD test to ChainSpecLoaderTests * Fix runner tests * Override MergePlugin Name and Description fields in AuRaMergePlugin * fix eth_call mix_hash and beneficiary (#4873) * fix eth_call mix_hash and beneficiary * always set MixHash # Conflicts: # src/Nethermind/Nethermind.Facade/BlockchainBridge.cs * Changes the docker password to a token (#4916) * fix: changes the docker password to token * fix: update docker pass for main release workflow * parallelized trace_filter execution per block * Remove unneded filters * Reduce traces memory allocation * fix trace filtering * fix whitespace * Updating Fast Sync config files (#4920) Co-authored-by: LukaszRozmej <LukaszRozmej@users.noreply.github.com> * Bump build version to 14.6 (#4923) Co-authored-by: Kamil Chodoła <kamil@nethermind.io> * Changes the docker password variable to token (#4924) * fix: changes the docker password to token * fix: update docker pass for main release workflow * fix: change DOCKER_PASSWORD to token * null action result when it is empty * Temporarily switch to `ubuntu-20.04` image (#4927) * Ignore ParityResult.IsEmpty in JSON * Fix remove empty results * Merge 1.14.7 * fix licensing header * fix whitespace * Final touches * Make ITraceSerializer generic * refactor * Adds IsTracingFees docs * FIx issue when Result was added to json, because it was created by default on deserialization, even if not serialized * fix shutdown if TraceStorePlugin not Enabled * Review fixes * Fix tests * fix shitespace Co-authored-by: Ruben Buniatyan <rubo@users.noreply.github.com> Co-authored-by: Kamil Chodoła <43241881+kamilchodola@users.noreply.github.com> Co-authored-by: Kamil Chodoła <kamil@nethermind.io> Co-authored-by: Jorge Mederos <46798594+jmederosalvarado@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: LukaszRozmej <LukaszRozmej@users.noreply.github.com> Co-authored-by: Oleg Jakushkin <oleg.jakushkin@gmail.com> Co-authored-by: Amirul Ashraf <asdacap@gmail.com> Co-authored-by: MarekM25 <MarekM25@users.noreply.github.com> Co-authored-by: Marek Moraczyński <marekm2504@gmail.com> Co-authored-by: Falco <1364936+FalcoXYZ@users.noreply.github.com>

OlegJakushkin added 3 commits October 24, 2022 12:18

Added 32 bytes check on extra data

a853213

Added checks and tests for the ExtraData not to be longer than 32 bytes

Code cleanup fixes

2d9e2fd

an empty line removed

Merge branch 'NethermindEth:master' into feature/ExtraDataChecks

bce0fbb

OlegJakushkin requested review from LukaszRozmej and MarekM25 October 24, 2022 11:28

MarekM25 reviewed Oct 24, 2022

View reviewed changes

OlegJakushkin requested a review from MarekM25 October 24, 2022 12:09

MarekM25 approved these changes Oct 24, 2022

View reviewed changes

OlegJakushkin merged commit c35e143 into NethermindEth:master Oct 24, 2022

kamilchodola pushed a commit that referenced this pull request Oct 24, 2022

Feature: added extra data length checks (#4805)

161d78e

* Added 32 bytes check on extra data Added checks and tests for the ExtraData not to be longer than 32 bytes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: added extra data length checks #4805

Feature: added extra data length checks #4805

OlegJakushkin commented Oct 24, 2022

MarekM25 Oct 24, 2022

OlegJakushkin Oct 24, 2022 •

edited

Loading

LukaszRozmej Oct 24, 2022 •

edited

Loading

MarekM25 left a comment

OlegJakushkin commented Oct 24, 2022 •

edited

Loading

Feature: added extra data length checks #4805

Feature: added extra data length checks #4805

Conversation

OlegJakushkin commented Oct 24, 2022

Changes:

Types of changes

Testing

MarekM25 Oct 24, 2022

Choose a reason for hiding this comment

OlegJakushkin Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

LukaszRozmej Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

MarekM25 left a comment

Choose a reason for hiding this comment

OlegJakushkin commented Oct 24, 2022 • edited Loading

OlegJakushkin Oct 24, 2022 •

edited

Loading

LukaszRozmej Oct 24, 2022 •

edited

Loading

OlegJakushkin commented Oct 24, 2022 •

edited

Loading