* Fix stability issues
why:
Handling malformed messages typically raises `RangeError` exceptions
when de-serialising RLP, or decoding message data. This is an
(incomplete) attempt to weed out some out it driven by real live
tests.
remark:
Employing the new `snap` protocol there might be different views on what
the messages really contain (currently specs are more a hint.)
* Update RLP exception handling
* Undo effect-less patch
why:
problem occurred somewhere above the try/catch handler
* Using `checkedEnumAssign()` for RLP enum
* 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>
* Handle the decodeAuthMessage error case separatly and log to trace
Garbage data on the TCP port (e.g. from port scanners) would
cause lots of error log messages, so log this to trace and get rid
of a (little) bit of exception usage in the process.
* Remove usage of result var in rlpxAccept and rlpxConnect
* Discv4: Add ENRRequest & ENRResponse msgs to avoid fails on these
Fix#499
These messages are not implemented yet however, but just ignored.
* disc: updateExternalIp()
New public proc that can be used to inform the discovery subsystem about
a changed external IP (as reported by UPnP/NAT-PMP in some other module).
why:
Rlp errors throw exceptions which cause the dispatcher loop to
terminate the current session immediately.
details:
The DisconnectionReasonList message requires a single entry list.
Observed and now accepted deviations are:
Geth: single byte number
bor(a Geth fork): blobbed single entry list containing a number
why:
This is a legacy feature and its usage should peter out over time.
details:
Use -d:chunked_rlpx_enabled for enabling chunked RLPx message handling.
why:
For some reason, Nethermind insists on sending chunked messages to
the syncing peer. Unfortunately, for the test networks the Nethermind
modes are the importent ones as they speak eth/65 as well while others
like Geth only support eth/66 which is not implemented here, yet.
The ENR code used to be solely exception based, and these
exceptions where a left-over of that. They are useless as later
calls use Result anyhow.
Additionally, they cause quite the performance loss because they
are used in the "common path" for the toTypedRecord call, e.g.
when reading the fields of ip6, tcp6 and udp6.
Currently only setting `--styleCheck:hint` as there are some
dependency fixes required and the compiler seems to trip over the
findnode MessageKind, findnode Message field and the findNode
proc. Also over protocol.Protocol usage.
Closes [nimbus-eth1#767](https://github.com/status-im/nimbus-eth1/issues/767).
Crashes occur when certain invalid RLPx messages are received from a peer.
Specifically, `msgId` out of range. Because any peer can easily trigger this
crash, we'd consider it a DOS vulnerability if Nimbus-eth1 was in general use.
We noticed when syncing to Goerli, there were some rare crashes with this
exception. It turned out one peer with custom code, perhaps malfunctioning,
was sending these messages if we were unlucky enough to connect to it.
`invokeThunk` is called from `dispatchMessages` and checks the range of
`msgId`. It correctly recognise that it's out of range, raises and exception
and produces a message. Job done.
Except the code in `dispatchMessage` treats all that as a warning instead of
error, and continues to process the message. A bit lower down, `msgId` is used
again without a range check.
The trivial fix is to check array bounds before access.
--
ps. Here's the stack trace ("reraised" sections hidden):
```
WRN 2021-11-08 21:29:33.238+00:00 Error while handling RLPx message topics="rlpx" tid=2003472 file=rlpx.nim:607 peer=Node[<IP>:45456] msg=45 err="RLPx message with an invalid id 45 on a connection supporting eth,snap"
/home/jamie/Status/nimbus-eth1/nimbus/p2p/chain/chain_desc.nim(437) main
/home/jamie/Status/nimbus-eth1/nimbus/p2p/chain/chain_desc.nim(430) NimMain
/home/jamie/Status/nimbus-eth1/nimbus/nimbus.nim(258) process
/home/jamie/Status/nimbus-eth1/vendor/nim-chronos/chronos/asyncloop.nim(279) poll
/home/jamie/Status/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(74) colonanonymous
/home/jamie/Status/nimbus-eth1/vendor/nim-eth/eth/p2p/rlpx.nim(1218) rlpxAccept
/home/jamie/Status/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(101) postHelloSteps
/home/jamie/Status/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(74) colonanonymous
/home/jamie/Status/nimbus-eth1/vendor/nim-eth/eth/p2p/rlpx.nim(985) postHelloSteps
/home/jamie/Status/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(101) dispatchMessages
/home/jamie/Status/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(77) colonanonymous
/home/jamie/Status/nimbus-eth1/vendor/nim-eth/eth/p2p/rlpx.nim(614) dispatchMessages
/home/jamie/Status/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/chcks.nim(23) raiseIndexError2
/home/jamie/Status/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(49) sysFatal
[[reraised from: ... ]]
[[reraised from: ... ]]
[[reraised from: ... ]]
[[reraised from: ... ]]
Error: unhandled exception: index 45 not in 0 .. 40 [IndexError]
```
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Separate the logging when the node is not reachable and
enrAutoUpdate is on or off to avoid confusion whether or not the
node might still become reachable.
queryRandom was currently only async for the `enrField` version.
However the basic queryRandom is also exported and thus gets
changed so it can be properly used as async proc.
Also added exports for the modules of which objects are used in
the discovery public API.
toBytes for NodeId wasn't selected by compiler byt if it does
get selected, it will fail on the test cases due to the
countdown that is done in logDistance.
Set to toBytesBE properly now and do countup, that should make
it correct also for BE architecture.
Removed toBytes to avoid confusion and avoid this one being
selected ever. The only place toBytes for NodeId was used is in
sessions.nim makeKey func and there also the stint one
(thus native endianness) was selected in Nim 1.2.x.
Native endianness is fine there as it is only an internal
representation.
* Don't fail ENR decoding when value is an RLP list
* Store RLP raw list in the ENR field pair instead
* Add ENR kList FieldKind so lists can be treated differently
Treated differently now when printing out the ENR, mentioning
that it is a raw RLP list
* Allow for tcp/udp ports to always be configured
- Allow for an ENR to be build with tcp and udp ports also when
no IP address is provided
- In the address set-up always provide best efforttcp and udp ports
also when configuration of external ip (and/or ports) fails.
* Modify nodes verification
* Move nodes verification to separate module
By moving verification to separate module it can be re-used
in different contexts not only in discoveryv5.
oskarth
Jacek Sieka
Jordan Hrycaj
Kim De Mey
Ștefan Talpalaru
Jordan Hrycaj
jangko
Zahary Karadjov
KonradStaniec
Etan Kissling
Jamie Lokier