Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions eth/enr/enr.nim
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ type
udp*: Opt[int]
tcp6*: Opt[int]
udp6*: Opt[int]
quic*: Opt[int]
quic6*: Opt[int]

EnrResult*[T] = Result[T, cstring]

Expand Down Expand Up @@ -218,7 +220,7 @@ macro initRecord*(
func insertAddress(
fields: var seq[FieldPair],
ip: Opt[IpAddress],
tcpPort, udpPort: Opt[Port]) =
tcpPort, udpPort, quicPort: Opt[Port]) =
## Insert address data.
## Incomplete address information is allowed (example: Port but not IP) as
## that information might be already in the ENR or added later.
Expand All @@ -233,24 +235,27 @@ func insertAddress(
fields.insert(("tcp", tcpPort.get().uint16.toField))
if udpPort.isSome():
fields.insert(("udp", udpPort.get().uint16.toField))
if quicPort.isSome():
fields.insert(("quic", quicPort.get().uint16.toField))

func init*(
T: type Record,
seqNum: uint64, pk: PrivateKey,
ip: Opt[IpAddress] = Opt.none(IpAddress),
tcpPort: Opt[Port] = Opt.none(Port),
udpPort: Opt[Port] = Opt.none(Port),
quicPort: Opt[Port] = Opt.none(Port),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way of adding a parameter does have the potential of breaking current usages. E.g. when extraFields is passed as parameter but not specifically set with extraFields = ....

But perhaps fine as we control most/all of its usage? Not sure.

extraFields: openArray[FieldPair] = []):
EnrResult[T] =
## Initialize a `Record` with given sequence number, private key, optional
## ip address, tcp port, udp port, and optional custom k:v pairs.
## ip address, tcp port, udp port, quic port, and optional custom k:v pairs.
##
## Can fail in case the record exceeds the `maxEnrSize`.
doAssert(not hasPredefinedKey(extraFields), "Predefined key in custom pairs")

var fields = newSeq[FieldPair]()

fields.insertAddress(ip, tcpPort, udpPort)
fields.insertAddress(ip, tcpPort, udpPort, quicPort)
fields.insert extraFields
makeEnrAux(seqNum, "v4", pk, fields)

Expand Down Expand Up @@ -326,16 +331,17 @@ func update*(
ip: Opt[IpAddress] = Opt.none(IpAddress),
tcpPort: Opt[Port] = Opt.none(Port),
udpPort: Opt[Port] = Opt.none(Port),
quicPort: Opt[Port] = Opt.none(Port),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem ^

extraFields: openArray[FieldPair] = []):
EnrResult[void] =
## Update a `Record` with given ip address, tcp port, udp port and optional
## custom k:v pairs.
## Update a `Record` with given ip address, tcp port, udp port, quic port
## and optional custom k:v pairs.
##
## If none of the k:v pairs are changed, the sequence number of the `Record`
## will still be incremented and a new signature will be applied.
##
## Providing an `Opt.none` for `ip`, `tcpPort` or `udpPort` will leave the
## corresponding field untouched.
## Providing an `Opt.none` for `ip`, `tcpPort`, `udpPort` or `quicPort` will
## leave the corresponding field untouched.
##
## Can fail in case of wrong `PrivateKey`, if the size of the resulting record
## exceeds `maxEnrSize` or if maximum sequence number is reached. The `Record`
Expand All @@ -349,7 +355,7 @@ func update*(
if pubkey.isNone() or pubkey.get() != pk.toPublicKey():
return err("Public key does not correspond with given private key")

r.pairs.insertAddress(ip, tcpPort, udpPort)
r.pairs.insertAddress(ip, tcpPort, udpPort, quicPort)
r.pairs.insert extraFields

if r.seqNum == high(type r.seqNum): # highly unlikely
Expand All @@ -376,7 +382,9 @@ func fromRecord*(T: type TypedRecord, r: Record): T =
tcp: r.tryGet("tcp", int),
tcp6: r.tryGet("tcp6", int),
udp: r.tryGet("udp", int),
udp6: r.tryGet("udp6", int)
udp6: r.tryGet("udp6", int),
quic: r.tryGet("quic", int),
quic6: r.tryGet("quic6", int)
)

func toTypedRecord*(r: Record): EnrResult[TypedRecord] {.deprecated: "Please use TypedRecord.fromRecord instead".} =
Expand Down Expand Up @@ -462,7 +470,7 @@ func fromBytesAux(T: type Record, s: openArray[byte]): EnrResult[T] =
of "secp256k1":
pkRaw = Opt.some rlpResult rlp.read(seq[byte])
pairs.add((k, Field(kind: kBytes, bytes: pkRaw.value())))
of "tcp", "udp", "tcp6", "udp6":
of "tcp", "udp", "tcp6", "udp6", "quic", "quic6":
let v = rlpResult rlp.read(uint16)
pairs.add((k, Field(kind: kNum, num: v)))
else:
Expand Down
3 changes: 2 additions & 1 deletion eth/p2p/discoveryv5/node.nim
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ func newNode*(r: Record): Result[Node, cstring] {.deprecated: "Use TypedRecord.f
func update*(n: Node, pk: PrivateKey, ip: Opt[IpAddress],
tcpPort: Opt[Port] = Opt.none(Port),
udpPort: Opt[Port] = Opt.none(Port),
quicPort: Opt[Port] = Opt.none(Port),
extraFields: openArray[FieldPair] = []): Result[void, cstring] =
? n.record.update(pk, ip, tcpPort, udpPort, extraFields)
? n.record.update(pk, ip, tcpPort, udpPort, quicPort, extraFields)

if ip.isSome():
if udpPort.isSome():
Expand Down
74 changes: 66 additions & 8 deletions eth/p2p/discoveryv5/protocol.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1059,7 +1059,7 @@ func init*(
proc newProtocol*(
privKey: PrivateKey,
enrIp: Opt[IpAddress],
enrTcpPort, enrUdpPort: Opt[Port],
enrTcpPort, enrUdpPort, enrQuicPort: Opt[Port],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also do without altering this one (afaik it is not needed for nimbus-eth2 and I'd like to deprecate it).

localEnrFields: openArray[FieldPair] = [],
bootstrapRecords: openArray[Record] = [],
previousRecord = Opt.none(enr.Record),
Expand All @@ -1083,14 +1083,14 @@ proc newProtocol*(
record = previousRecord.get()
# TODO: this is faulty in case the intent is to remove a field with
# opt.none
record.update(privKey, enrIp, enrTcpPort, enrUdpPort,
record.update(privKey, enrIp, enrTcpPort, enrUdpPort, enrQuicPort,
localEnrFields).expect("Record within size limits and correct key")
else:
record = enr.Record.init(1, privKey, enrIp, enrTcpPort, enrUdpPort,
record = enr.Record.init(1, privKey, enrIp, enrTcpPort, enrUdpPort, enrQuicPort,
localEnrFields).expect("Record within size limits")

info "Discovery ENR initialized", enrAutoUpdate, seqNum = record.seqNum,
ip = enrIp, tcpPort = enrTcpPort, udpPort = enrUdpPort,
ip = enrIp, tcpPort = enrTcpPort, udpPort = enrUdpPort, quicPort = enrQuicPort,
localEnrFields, uri = toURI(record)
if enrIp.isNone():
if enrAutoUpdate:
Expand Down Expand Up @@ -1124,6 +1124,26 @@ proc newProtocol*(
privKey: PrivateKey,
enrIp: Opt[IpAddress],
enrTcpPort, enrUdpPort: Opt[Port],
localEnrFields: openArray[FieldPair] = [],
bootstrapRecords: openArray[Record] = [],
previousRecord = Opt.none(enr.Record),
bindPort: Port,
bindIp = IPv4_any(),
enrAutoUpdate = false,
banNodes = false,
config = defaultDiscoveryConfig,
rng = newRng()):
Protocol =
newProtocol(
privKey, enrIp, enrTcpPort, enrUdpPort, Opt.none(Port), localEnrFields,
bootstrapRecords, previousRecord, bindPort, bindIp, enrAutoUpdate,
banNodes, config, rng
)

proc newProtocol*(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the diff between the two newProtocol procs? Maybe we could have a comment there, or maybe one can be private. Or maybe we could have different proc names for the brand new procs, e.g., newProtocolABC so that we have simpler lookups.

Copy link
Copy Markdown
Contributor

@kdeme kdeme Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is the bindIp parameter.

One accepts an IpAddress, the other (newer) one an Opt[IpAddress].
Functionally mostly the same, but with the important (and rather hidden) difference that passing a None there has the effect that this also gets passed down to the newDatagramTransport which underneath looks up for the appropriate "AnyAddress" (:: when dual stack, 0.0.0.0 when IPv4 only).

I want to deprecate the original one, and keep just the newer one with proper comment explaining this.

privKey: PrivateKey,
enrIp: Opt[IpAddress],
enrTcpPort, enrUdpPort, enrQuicPort: Opt[Port],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idem for this one

localEnrFields: openArray[(string, seq[byte])] = [],
bootstrapRecords: openArray[Record] = [],
previousRecord = Opt.none(enr.Record),
Expand All @@ -1136,7 +1156,7 @@ proc newProtocol*(
): Protocol =
let customEnrFields = mapIt(localEnrFields, toFieldPair(it[0], it[1]))
newProtocol(
privKey, enrIp, enrTcpPort, enrUdpPort, customEnrFields, bootstrapRecords,
privKey, enrIp, enrTcpPort, enrUdpPort, enrQuicPort, customEnrFields, bootstrapRecords,
previousRecord, bindPort, bindIp, enrAutoUpdate, banNodes, config, rng,
)

Expand All @@ -1148,6 +1168,26 @@ proc newProtocol*(
bootstrapRecords: openArray[Record] = [],
previousRecord = Opt.none(enr.Record),
bindPort: Port,
bindIp = IPv4_any(),
enrAutoUpdate = false,
banNodes = false,
config = defaultDiscoveryConfig,
rng = newRng(),
): Protocol =
newProtocol(
privKey, enrIp, enrTcpPort, enrUdpPort, Opt.none(Port), localEnrFields,
bootstrapRecords, previousRecord, bindPort, bindIp, enrAutoUpdate, banNodes,
config, rng
)

proc newProtocol*(
privKey: PrivateKey,
enrIp: Opt[IpAddress],
enrTcpPort, enrUdpPort, enrQuicPort: Opt[Port],
localEnrFields: openArray[(string, seq[byte])] = [],
bootstrapRecords: openArray[Record] = [],
previousRecord = Opt.none(enr.Record),
bindPort: Port,
bindIp: Opt[IpAddress],
enrAutoUpdate = false,
config = defaultDiscoveryConfig,
Expand All @@ -1159,15 +1199,15 @@ proc newProtocol*(
var res = previousRecord.get()
# TODO: this is faulty in case the intent is to remove a field with
# opt.none
res.update(privKey, enrIp, enrTcpPort, enrUdpPort,
res.update(privKey, enrIp, enrTcpPort, enrUdpPort, enrQuicPort,
customEnrFields).expect("Record within size limits and correct key")
res
else:
enr.Record.init(1, privKey, enrIp, enrTcpPort, enrUdpPort,
enr.Record.init(1, privKey, enrIp, enrTcpPort, enrUdpPort, enrQuicPort,
customEnrFields).expect("Record within size limits")

info "Discovery ENR initialized", enrAutoUpdate, seqNum = record.seqNum,
ip = enrIp, tcpPort = enrTcpPort, udpPort = enrUdpPort,
ip = enrIp, tcpPort = enrTcpPort, udpPort = enrUdpPort, quicPort = enrQuicPort,
customEnrFields, uri = toURI(record)

if enrIp.isNone():
Expand Down Expand Up @@ -1198,6 +1238,24 @@ proc newProtocol*(
responseTimeout: config.responseTimeout,
rng: rng)

proc newProtocol*(
privKey: PrivateKey,
enrIp: Opt[IpAddress],
enrTcpPort, enrUdpPort: Opt[Port],
localEnrFields: openArray[(string, seq[byte])] = [],
bootstrapRecords: openArray[Record] = [],
previousRecord = Opt.none(enr.Record),
bindPort: Port,
bindIp: Opt[IpAddress],
enrAutoUpdate = false,
config = defaultDiscoveryConfig,
rng = newRng()): Protocol =
newProtocol(
privKey, enrIp, enrTcpPort, enrUdpPort, Opt.none(Port), localEnrFields,
bootstrapRecords, previousRecord, bindPort, bindIp, enrAutoUpdate,
config, rng
)

proc `$`*(a: OptAddress): string =
if a.ip.isNone():
"*:" & $a.port
Expand Down
6 changes: 4 additions & 2 deletions tests/fuzzing/enr/generate.nim
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ proc generate() =
port = Opt.some(Port(20301))

block:
let record = enr.Record.init(1, privKey, ip, port, port)[]
let record = enr.Record.init(1, privKey, ip, port, port, port)[]
record.raw.toFile(inputsDir / "enr1")
block:
let record = enr.Record.init(1, privKey, ip, port, port, [toFieldPair("test", 1'u)])[]
let record = enr.Record.init(
1, privKey, ip, port, port, port, [toFieldPair("test", 1'u)]
)[]
record.raw.toFile(inputsDir / "enr2")

discard existsOrCreateDir(inputsDir)
Expand Down
6 changes: 4 additions & 2 deletions tests/p2p/discv5_test_helper.nim
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ proc initDiscoveryNode*(
let protocol = newProtocol(
privKey,
Opt.some(address.ip),
Opt.some(address.port), Opt.some(address.port),
Opt.some(address.port),
Opt.some(address.port),
Opt.some(address.port),
bindPort = address.port,
bootstrapRecords = bootstrapRecords,
localEnrFields = localEnrFields,
Expand All @@ -54,7 +56,7 @@ func generateNode*(privKey: PrivateKey, port: int = 20302,
localEnrFields: openArray[FieldPair] = []): Node =
let port = Port(port)
let enr = enr.Record.init(1, privKey, Opt.some(ip),
Opt.some(port), Opt.some(port), localEnrFields).expect("Properly initialized private key")
Opt.some(port), Opt.some(port), Opt.some(port), localEnrFields).expect("Properly initialized private key")
result = Node.fromRecord(enr)

proc generateNRandomNodes*(rng: var HmacDrbgContext, n: int): seq[Node] =
Expand Down
14 changes: 8 additions & 6 deletions tests/p2p/test_discoveryv5.nim
Original file line number Diff line number Diff line change
Expand Up @@ -428,15 +428,17 @@ suite "Discovery v5.1 Tests":
privKey = PrivateKey.random(rng[])
ip = Opt.some(parseIpAddress("127.0.0.1"))
port = Port(20301)
node = newProtocol(privKey, ip, Opt.some(port), Opt.some(port), bindPort = port,
rng = rng)
node = newProtocol(privKey, ip, Opt.some(port), Opt.some(port),
Opt.some(port), bindPort = port, rng = rng)
noUpdatesNode = newProtocol(privKey, ip, Opt.some(port), Opt.some(port),
bindPort = port, rng = rng, previousRecord = Opt.some(node.getRecord()))
Opt.some(port), bindPort = port, rng = rng,
previousRecord = Opt.some(node.getRecord()))
updatesNode = newProtocol(privKey, ip, Opt.some(port), Opt.some(Port(20302)),
bindPort = port, rng = rng,
Opt.some(port), bindPort = port, rng = rng,
previousRecord = Opt.some(noUpdatesNode.getRecord()))
moreUpdatesNode = newProtocol(privKey, ip, Opt.some(port), Opt.some(port),
bindPort = port, rng = rng, localEnrFields = {"addfield": @[byte 0]},
Opt.some(port), bindPort = port, rng = rng,
localEnrFields = {"addfield": @[byte 0]},
previousRecord = Opt.some(updatesNode.getRecord()))
check:
node.getRecord().seqNum == 1
Expand All @@ -447,7 +449,7 @@ suite "Discovery v5.1 Tests":
# Defect (for now?) on incorrect key use
expect ResultDefect:
discard newProtocol(PrivateKey.random(rng[]),
ip, Opt.some(port), Opt.some(port), bindPort = port, rng = rng,
ip, Opt.some(port), Opt.some(port), Opt.some(port), bindPort = port, rng = rng,
previousRecord = Opt.some(updatesNode.getRecord()))

asyncTest "Update node record with revalidate":
Expand Down
Loading