Don't treat 3 characters as special in `cmp`. `cmp` for `ProtocolInfo` was
wrong because it ignored all characters after the first 3.
In the wild we have seen protocol names longer than 3 characters. `snap`,
`hive`, `istanbul`, `bzzeth`, `bzz-stream`, `bzz-retrieve`, `dbix`, `opera`,
`pchain`, `pchain_child_0`, `sero`, `smilobft`, `spock`.
There was never a 3 character limit in the [specification]
(https://github.com/ethereum/devp2p/blob/master/rlpx.md).
It always said "short ASCII name", until recently on 2021-02-25 it was changed
to an 8 characters limit.
Also `pi.nameStr` can be removed. Nothing uses it, and it has the same actual
effect as just copying the string `pi.name`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
For a long time this caused invalid RLP parsing of `NewBlock` messages in the
`eth` protocol.
The `rlpInline` pragma was accepted but had no effect. We could implemented
it, but it doesn't seem worth doing, with tests etc, as there's only one user
which has been fixed another way.
With `NewBlock`, whenever a peer sent us `NewBlock`, we'd get an RLP decoding
error, and disconnected the peer thinking it was the peer's error.
These messages are sent often by good peers, so whenever we connected to a
really good peer, we'd end up disconnecting within a minute due to this. This
went unnoticed for years, as we stayed connected to old peers which have no new
blocks, and we weren't looking at peer quality, disconnect reasons or real-time
blockchain updates anyway.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
1. Generalises the special cases for serialising RLP `seq[Transaction]`.
Previously it only used the special case inside `BlockBody` and `EthBlock`.
Now it uses it for all `seq[Transaction]` regardless of what objects they
are parts of, or no object at all. `openArray[Transaction]` is also
included, as this was found to be necessary to match in some places.
2. Bug fix parsing `Transaction`: Always read the first byte to get the
transaction type instead of parsing an RLP `int`. This way invalid or
adversarial input gives a correct error (i.e. invalid type code).
When it was read with `rlp.read(int)`, those inputs gave many crazy
messages (e.g. "too large to fit in memory"). In the specification it's a
byte. (Technically the input is not RLP and we shouldn't be using the RLP
parser anyway to parse standalone transaction objects).
3. Bug fix parsing `Transaction`: If a typed transaction is detected in
`seq[Transaction]`, the previous code removed the RLP (blob) wrapper, then
passed the contents to `read(Transaction)`. That meant a blob-wrapped
legacy transaction would be accepted. This is incorrect. The new code
passes the contents to the typed transaction decoder, which correctly
rejects a wrapped legacy transaction as having invalid type.
Change 1 has a large, practical effect on `eth/65` syncing with peers.
Serialisation of `eth` message types `Transactions` and `PooledTransactions`
have been broken since the introduction of typed transactions (EIP-2718), as
used in Berlin/London forks. (The special case for `seq[Transaction]` inside
`BlockBody` only fixed message type `BlockBodies`.)
Due to this, whenever a peer sent us a `Transactions` message, we had an RLP
decoding error processing it, and disconnected the peer thinking it was the
peer's error.
These messages are sent often by good peers, so whenever we connected to a
really good peer, we'd end up disconnecting from it within a few tens of
seconds due to this.
This didn't get noticed before updating to `eth/65`, because with old protocols
we tend to only connect to old peers, which may be out of date themselves and
have no typed transactions. Also, we didn't really investigate occasional
disconnects before, we assumed they're just part of P2P life.
The root cause is the RLP serialisation of individual `Transaction` is meant to
be subtly different from arrays/sequences of `Transaction` objects in network
messages. RFC-2976 covers this but it's quite subtle:
- Individual transactions are encoded and stored as either `RLP([fields..])`
for legacy transactions, or `Type || RLP([fields..])`. Both of these
encodings are byte sequences. The part after `Type` doesn't have to be
RLP in theory, but all types so far use RLP. EIP-2718 covers this.
- In arrays (sequences), transactions are encoded as either `RLP([fields..])`
for legacy transactions, or `RLP(Type || RLP([fields..]))` for all typed
transactions to date. Spot the extra `RLP(..)` blob encoding, to make it
valid RLP inside a larger RLP. EIP-2976 covers this, "Typed Transactions
over Gossip", although it's not very clear about the blob encoding.
In practice the extra `RLP(..)` applies to all arrays/sequences of transactions
that are to be RLP-encoded as a list. In principle, it should be all
aggregates (object fields etc.), but it's enough for us to enable it for all
arrays/sequences, as this is what's used in the protocol and EIP-2976.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Storing large blobs in a "WITHOUT ROWID" table turns out to be extremely
slow when the tree must be rebalanced.
* Split out keystore capability into separate interface, making each
keystore a separate instance
* Disable "WITHOUT ROWID" optimization by default
* Implement prefix lookup that allows iterating over all values with a
certain prefix in their key
* Fix raw Exceptions in hexary caused by forward declarations
* Fix raw Exceptions in trie/db caused by forward declarations
* And now we can remove those db Proc CatchableError raises
`les_protocol.nim` failed to build, due to very silly Nim bugs
nim-lang/Nim#8792 and nim-lang/Nim#17102.
import
../../[rlp, keys], ../../common/eth_types,
../[rlpx, kademlia, blockchain_utils], ../private/p2p_types,
The silly part is `../` has to be quoted if it's before a group of files, but
not before a single file. Most places in PR #344 / 7624153 use the workaround
`".."/` but it was missed in `les_protocol.nim`:
nimbus-eth1/vendor/nim-eth/eth/p2p/rlpx_protocols/les_protocol.nim(14, 3)
Error: cannot open file: ../../[rlp,keys]
Signed-off-by: Jamie Lokier <jamie@shareable.org>
It's been confirmed that the test fails if the Kademlia crash workaround isn't
in `findNode`, and passes with it there.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Jamie Lokier
KonradStaniec
Kim De Mey
Ștefan Talpalaru
jangko
Jacek Sieka