Skip to content
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.

Sign up for GitHub

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

Cache and resend, rather than recreate, builder API registrations #4040

Merged
merged 3 commits into from
Aug 31, 2022
Merged

Cache and resend, rather than recreate, builder API registrations #4040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e50128
Cache and resend, rather than recreate, builder API registrations
tersec Aug 26, 2022
cf539c9
copy attached validator keys in function to avoid async data race
tersec Aug 29, 2022
2629ac3
check before every access
tersec Aug 29, 2022
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
1 change: 0 additions & 1 deletion beacon_chain/consensus_object_pools/block_dag.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ else:
{.push raises: [].}

import
std/options,
chronicles,
../spec/datatypes/[phase0, altair, bellatrix],
../spec/forks
Expand Down
2 changes: 1 addition & 1 deletion beacon_chain/consensus_object_pools/blockchain_dag.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1952,7 +1952,7 @@ proc preInit*(
tailStateSlot = getStateField(tailState, slot)

let genesisBlockRoot = withState(genesisState):
if state.root != getStateRoot(tailState):
if forkyState.root != getStateRoot(tailState):
# Different tail and genesis
if state.data.slot >= getStateField(tailState, slot):
fatal "Tail state must be newer or the same as genesis state"
Expand Down
2 changes: 1 addition & 1 deletion beacon_chain/spec/beaconstate.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ else:
{.push raises: [].}

import
std/[algorithm, collections/heapqueue, math, options, sequtils, tables],
std/[algorithm, collections/heapqueue, math, sequtils, tables],
stew/assign2,
json_serialization/std/sets,
chronicles,
Expand Down
4 changes: 2 additions & 2 deletions beacon_chain/spec/state_transition_block.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ else:
{.push raises: [].}

import
std/[algorithm, options, sequtils, sets, tables],
std/[algorithm, sequtils, sets, tables],
chronicles, metrics,
../extras,
./datatypes/[phase0, altair, bellatrix],
Expand Down Expand Up @@ -172,7 +172,7 @@ proc check_proposer_slashing*(
state: var ForkedHashedBeaconState; proposer_slashing: SomeProposerSlashing;
flags: UpdateFlags): Result[ValidatorIndex, cstring] =
withState(state):
check_proposer_slashing(state.data, proposer_slashing, flags)
check_proposer_slashing(forkyState.data, proposer_slashing, flags)

# https://github.com/ethereum/consensus-specs/blob/v1.2.0-rc.3/specs/phase0/beacon-chain.md#proposer-slashings
proc process_proposer_slashing*(
Expand Down
39 changes: 30 additions & 9 deletions beacon_chain/validators/validator_duties.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,6 +1280,8 @@ proc getValidatorRegistration(

return ok validatorRegistration

from std/sequtils import toSeq

proc registerValidators(node: BeaconNode, epoch: Epoch) {.async.} =
try:
if (not node.config.payloadBuilderEnable) or
Expand All @@ -1303,10 +1305,20 @@ proc registerValidators(node: BeaconNode, epoch: Epoch) {.async.} =
builderStatus = restBuilderStatus
return

# TODO cache the generated registrations and keep resending the previous ones
# The async aspect of signing the registrations can cause the attached
# validators to change during the loop.
let attachedValidatorPubkeys =
toSeq(node.attachedValidators[].validators.keys)

# https://github.com/ethereum/builder-specs/blob/v0.2.0/specs/validator.md#validator-registration
var validatorRegistrations: seq[SignedValidatorRegistrationV1]
for validator in node.attachedValidators[].validators.values:
for key in attachedValidatorPubkeys:
# Time passed during awaits; REST keymanager API might have removed it
if key notin node.attachedValidators[].validators:
continue

let validator = node.attachedValidators[].validators[key]

if validator.index.isNone:
continue

Expand All @@ -1321,14 +1333,23 @@ proc registerValidators(node: BeaconNode, epoch: Epoch) {.async.} =
state.data.validators.item(validator.index.get).exit_epoch:
continue

let validatorRegistration =
await node.getValidatorRegistration(validator, epoch)
if validatorRegistration.isErr:
error "registerValidators: validatorRegistration failed",
validatorRegistration
continue
if validator.externalBuilderRegistration.isSome:
validatorRegistrations.add validator.externalBuilderRegistration.get
else:
let validatorRegistration =
await node.getValidatorRegistration(validator, epoch)
Copy link
Member

Choose a reason for hiding this comment

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

if someone adds a validator via rest api, I'm guessing this await can get interleaved (cc @zah?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, there are lots of the usual kinds approaches people use when modifying sequences in place, but for this purpose, it mostly matters whether table mvalues iteration works with that. It's fine if some new validator is missed this epoch around in that scenario, and it's even fine to just abort the table scan in a given epoch if there's any issues of this sort. I'd prefer either, ideally, to adding more internal buffering to this function of information on all attached validators, but that could, ultimately, solve this.

Copy link
Contributor

@zah zah Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The problem here is that the contents of the validators table may change while you are iterating over it. This would be detected as a Defect by the standard library.

The standard work-around is to copy the list of validators prior to starting the iteration by iterating over dup(node.attachedValidators[].validators.values).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cf539c9 works around this. Rather than copy the values in their entirety, which would be a better approach for a read-only application, here it copies the keys, because it's more obviously symmetrical that the access mechanism that works for reading also works for writing.

if validatorRegistration.isErr:
error "registerValidators: validatorRegistration failed",
validatorRegistration
continue

# Time passed during await; REST keymanager API might have removed it
if key notin node.attachedValidators[].validators:
continue

validatorRegistrations.add validatorRegistration.get
node.attachedValidators[].validators[key].externalBuilderRegistration =
Opt.some validatorRegistration.get
validatorRegistrations.add validatorRegistration.get

let registerValidatorResult =
awaitWithTimeout(node.payloadBuilderRestClient.registerValidator(validatorRegistrations),
Expand Down
22 changes: 15 additions & 7 deletions beacon_chain/validators/validator_pool.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ type

# Cache the latest slot signature - the slot signature is used to determine
# if the validator will be aggregating (in the near future)
slotSignature*: Option[tuple[slot: Slot, signature: ValidatorSig]]
slotSignature*: Opt[tuple[slot: Slot, signature: ValidatorSig]]

# For the external payload builder; each epoch, the external payload
# builder should be informed of current validators
externalBuilderRegistration*: Opt[SignedValidatorRegistrationV1]

startSlot*: Slot

Expand Down Expand Up @@ -93,8 +97,10 @@ proc addLocalValidator*(pool: var ValidatorPool, item: KeystoreData,
index: Opt[ValidatorIndex], slot: Slot) =
doAssert item.kind == KeystoreKind.Local
let pubkey = item.pubkey
let v = AttachedValidator(kind: ValidatorKind.Local, pubkey: pubkey,
index: index, data: item, startSlot: slot)
let v = AttachedValidator(
kind: ValidatorKind.Local, pubkey: pubkey, index: index, data: item,
externalBuilderRegistration: Opt.none SignedValidatorRegistrationV1,
startSlot: slot)
pool.validators[pubkey] = v
notice "Local validator attached", pubkey, validator = shortLog(v),
start_slot = slot
Expand All @@ -109,9 +115,11 @@ proc addRemoteValidator*(pool: var ValidatorPool, item: KeystoreData,
index: Opt[ValidatorIndex], slot: Slot) =
doAssert item.kind == KeystoreKind.Remote
let pubkey = item.pubkey
let v = AttachedValidator(kind: ValidatorKind.Remote, pubkey: pubkey,
index: index, data: item, clients: clients,
startSlot: slot)
let v = AttachedValidator(
kind: ValidatorKind.Remote, pubkey: pubkey, index: index, data: item,
clients: clients,
externalBuilderRegistration: Opt.none SignedValidatorRegistrationV1,
startSlot: slot)
pool.validators[pubkey] = v
notice "Remote validator attached", pubkey, validator = shortLog(v),
remote_signer = $item.remotes,
Expand Down Expand Up @@ -403,7 +411,7 @@ proc getSlotSignature*(v: AttachedValidator, fork: Fork,
if signature.isErr:
return signature

v.slotSignature = some((slot, signature.get))
v.slotSignature = Opt.some((slot, signature.get))
return signature

# https://github.com/ethereum/builder-specs/blob/v0.2.0/specs/builder.md#signing
Expand Down
2 changes: 1 addition & 1 deletion research/simutils.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ proc loadGenesis*(validators: Natural, validate: bool):
quit 1

if forkyState.data.validators.len != validators:
echo &"Supplied genesis file has {state.data.validators.len} validators, while {validators} where requested, running anyway"
echo &"Supplied genesis file has {forkyState.data.validators.len} validators, while {validators} where requested, running anyway"

echo &"Loaded {genesisFn}..."

Expand Down
8 changes: 4 additions & 4 deletions research/state_sim.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ cli do(slots = SLOTS_PER_EPOCH * 5,
write(stdout, ".")

if last:
withState(state[]): writeJson("state.json", state.data)
withState(state[]): writeJson("state.json", forkyState.data)
else:
withState(state[]):
if state.data.slot mod json_interval.uint64 == 0:
writeJson(jsonName(prefix, state.data.slot), state.data)
if forkyState.data.slot mod json_interval.uint64 == 0:
writeJson(jsonName(prefix, forkyState.data.slot), forkyState.data)
write(stdout, ":")
else:
write(stdout, ".")
Expand Down Expand Up @@ -107,7 +107,7 @@ cli do(slots = SLOTS_PER_EPOCH * 5,

withState(state[]):
let
slot = state.data.slot
slot = forkyState.data.slot
epoch = slot.epoch
committees_per_slot =
get_committee_count_per_slot(state.data, epoch, cache)
Expand Down