-
Notifications
You must be signed in to change notification settings - Fork 432
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
Dns peer feed no queue #3716
Dns peer feed no queue #3716
Conversation
… with master node
… from the mock - also found one bug with session that was fixed by the changes
src/Nethermind/Nethermind.Network.Benchmark/NodeStatsCtorBenchmarks.cs
Outdated
Show resolved
Hide resolved
string broken = nodeRecordText[4..].Replace("-", "+").Replace("_", "/"); | ||
broken = broken.PadRight(nodeRecordText.Length % 4); | ||
RlpStream rlpStream = new(Convert.FromBase64String(broken)); |
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 looks weird, also string manipulation can prrobably in place char span manipulation
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.
impact on memory is minimal, measured it in dotmemory 16kb or so
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.
Is the PadRight doing what is expected here?
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.
Moved it to a bit better implementation with StringBuilder, still not sure about PadRight
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 needs tests to be added!
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.
Fixed a lot of things but EnrDiscoveryTests fails to parse 1/6th of enr's
src/Nethermind/Nethermind.Runner/Properties/launchSettings.json
Outdated
Show resolved
Hide resolved
// if (publicKeyOutput.Length < 64) | ||
// { | ||
// throw new ArgumentException($"{nameof(publicKeyOutput)} must be {64} bytes"); | ||
// } |
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.
Is this safe?
// int expectedInputLength = flags == Secp256K1EcCompressed ? 33 : 64; | ||
// if (publicKey.Length != expectedInputLength) | ||
// { | ||
// throw new ArgumentException($"{nameof(publicKey)} must be {expectedInputLength} bytes"); | ||
// } |
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.
Is this safe?
# Conflicts: # src/Nethermind/Nethermind.Trie/TrieNode.cs
Fixes | Closes | Resolves #
Changes:
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests??
Comments about testing , should you have some (optional)
Further comments (optional)
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...