The assert would occur when the rlp size of the a node in the
nodes rlp list is incorrectly set too high and then the next
`listElem` call for the next node will start from the
incorrect data. When that data is not a list the assert in
`listElem` will be triggered.
Fixed by adding a `listLen` call which checks if it is a list.
Added also more strictness by:
- Checking if that list is of len 4, which it must be
- raising immediatly on invalid IP length
- raising immediatly on invalid public key / node id
+ test cases
* drop support for v4 (obsolete, doesn't work with all clients since
they use chunking and other obsolete v4 features that we're missing or
don't support it at all)
* rework asyncraises
* always store generated p2p macro code (similar to eth2)
* preparation for chronos cancellation support (more to be done here)
* when peer is disconnected, ensure pending handshakes and requests are
notified (instead of waiting for timeout)
* disallow raising from `onPeerDisconnected` - this simplifies
disconnection coordination among async tasks
* introduce several warning logs for protocol breaches - these should be
removed eventually, pending q/a on the rlpx layer in general
* fix snappy compression - the payload without msgId should be
compressed
* remove strict checks on unused fields in RLPx message header (this
matches geth behavior and the spirit of EIP-8)
* add snappy dep
* disconnect peers that send non-hello messages during initial hello
step
* fix devp2p protocol version - 4 because we don't implement snappy
(yet) - this is cosmetic since this particular version field is not
actually being used
* fix ack message length checking
* move RLPx transport code to separate module, annotate with asyncraises
* increase max RLPx message size to 16mb, per EIP-706
* make sure both accept/connect timeout after 10s
* aim to log every connection attempt once at debug level
* make capability-id/context-id check more accurate
* disallow random messages before hello
Support for previous versions was
[removed](7194c847b6)
from geth in 2021 after other clients had migrated - should be safe to
remove here also.
* fix snappy detection - it should use the hello version, not the RLPx
handshake
* simplify generation of auth messages
* pad 100-300 bytes like spec suggests
TODO: error handling is all over the place - will be addressed in a
follow-up PR
As it happens, the two share the exact same interface (even the test
suite removed in this PR passes) - `minilru` has an edge on efficiency
however, avoiding the doubly linked list node allocations etc
Since these types were written, we've gained an executable spec:
https://github.com/ethereum/execution-specs
This PR aligns some of the types we use with this spec to simplify
comparisons and cross-referencing.
Using a `distinct` type is a tradeoff between nim ergonomics, type
safety and the ability to work around nim quirks and stdlib weaknesses.
In particular, it allows us to overload common functions such as `hash`
with correct and performant versions as well as maintain control over
string conversions etc at the cost of a little bit of ceremony when
instantiating them.
Apart from distinct byte types, `Hash32`, is introduced in lieu of the
existing `Hash256`, again aligning this commonly used type with the spec
which picks bytes rather than bits in the name.
* Using unsigned types for message type and requst IDs
why:
Negative values are neither defined for RLP nor in the protocol specs
which refer to the RLPs (see yellow paper app B clause (199).
* Fix `int` argument (must be `uint`) in fuzzing tests
why:
Not part of all tests so it slipped through.
- Rework to have exception raise only at rlp decoding and use
result types from then onwards
- Adjust the current API to have result versions and deprecated
the ones which had var Record + bool
- Add PublickKey to the Record object, as this allows us to skip
fromRaw calls whenever access is needed to the public key
- Add a TypedRecord.fromRecord which cannot fail and deprecate
the old one
- Some other minor clean-up & re-ordering
- Rework adding and updating of fields by having an insert call
that gets used everywhere. Avoiding also duplicate keys. One
side-effect of this is that ENR sequence number will always get
updated on an update call, even if nothing changes.
- Deprecate initRecord as it is only used in tests and is flawed
- Assert when predefined keys go into the extra custom pairs.
Any of the predefined keys are only to be passed now via specific
parameters to make sure that the correct types are stored in ENR.
- Clearify the Opt.none behaviour for Record.update
- When setting ipv6, allow for tcp/udp port fields to be used
default
- General clean-up
- Rework/clean-up completely the ENR tests.
A first step in cleaning up RLP, which has lots of interesting issues -
the next step would be to clean up the exception handling as well
(Resultify?)
* remove `RlpNode` (unused)
* single-pass parsing for most functionality via RlpItem
* stricter conformance to spec
* remove float support
* warn about signed integers
* raise on invalid RLP earlier
* avoid several pointless allocations, in particular in `listLen`,
`listElem` etc
* include spec docs
* Refactor p2pProtocol internals
* Attempt to fix rlp crash with Nim v2 (#658)
* Attempt to fix rlp crash with Nim v2
* Fix test_ecies for nim v2
* Reduce compiler warnings
* Resolve ambiquity in testutils
* Disable nim devel continue-on-error
This allows for protocols build on top of discv5 to use the ENR
provided in the handshake directly, instead of having to rely
on requesting it from the discv5 routing table.
* Add metrics related to devp2p peer connections
* Avoid reconnecting to peers that just failed connection
- Add SeenTable to avoid reconnecting to peers immediately after
a failed connect. Depending on the failure, the amount of time is
different. This is similar to what is done in nimbus-eth2.
- Attempt to rework rlpxConnect at the same time, in order to
make sure that errors are properly handled. The current structure
is far from ideal, but it is hopefully a small step in the right
direction. To many oddities in there right now to really rework
rlpxConnect properply.
* Fix rlpx thunk fuzzer
- Fix two possible RangeErrors, due to negative seq allocation
in decodeAuthMessageEIP8 and in decodeAckMessageEIP8
- Set the minimum auth message size to AuthMessageEIP8Length,
in case there are clients that no longer add padding
- Add tests for invalid length cases
RLP Enum deserialization would currently not check if "hole values"
were attempted to be converted to the enum type. Now use
checkedEnumAssign and fail with RlpTypeMismatch on invalid values.
There is at least one occurance of an enum with holes in rlpx p2p:
DisconnectionReason. For this enum the issue could occur.
Also:
- Added enum RLP tests and rlpx p2p disconnect message tests to
test the DisconnectionReason with enum hole value.
- Fixed worse custom DisconnectionReason decoding occurance
in rlpx in waitSingleMsg proc where this issue could occur.
Nim devel brach(1.7.1) introduce gc=orc as default mode.
Because the p2p protocol using unsafe pointer operations
for it's ProtocolInfo and using global variables scattered
around, the orc mistakenly(or maybe correctly) crash the protocol.
* Provide dedicated `DisconnectionReason` enum type RLP reader
why:
Without this reader, the program communicating via RLPX will crash when
receiving out of bound reason codes disconnect message.
Out of bound value assignments to an enum causes a `RangeError`defect
and consequently the program to terminate. This `RangeError` is avoided
here and a `MalformedRlpError` catchable error raised.
* Using default exception type in bespoke `read(DisconnectionReason)`
why:
This should not differ from the default enum parser. The particular
message is different and more targeted, here.
Note: The default RLP parser was not used because `
`array[1,DisconnectionReason]` is currently not properly handled and
should give a siliar error message as a `DisconnectionReason` error.
* De-clutter, custom read() was not needed
Co-authored-by: jordan <jordan@curd.mjh-it.com>
Kim De Mey
Jacek Sieka
Csaba Kiraly
tersec
Kim De Mey
Jordan Hrycaj
andri lim
Justin Traglia
KonradStaniec
oskarth
Jordan Hrycaj