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.

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

more fork-choice fixes #1388

Merged
merged 4 commits into from
Jul 30, 2020
Merged

more fork-choice fixes #1388

merged 4 commits into from
Jul 30, 2020

Conversation

arnetheduck
Copy link
Member

  • use target block/epoch to validate attestations
  • make addLocalValidators sync
  • add current and previous epoch to cache before doing state transition
  • update head state using clearance state as a shortcut, when possible
  • use blockslot for fork choice balances
  • send attestations using epochref cache

@tersec
Copy link
Contributor

tersec commented Jul 28, 2020

Rebase to fix the build: #1389

@arnetheduck
Copy link
Member Author

green now on travis @tersec

@arnetheduck arnetheduck requested a review from tersec July 30, 2020 12:56
* use target block/epoch to validate attestations
* make addLocalValidators sync
* add current and previous epoch to cache before doing state transition
* update head state using clearance state as a shortcut, when possible
* use blockslot for fork choice balances
* send attestations using epochref cache
also simplify epoch block traversal
@arnetheduck arnetheduck force-pushed the fork-choice-fixes-6 branch from c768813 to 65f93b3 Compare July 30, 2020 13:41
@arnetheduck arnetheduck force-pushed the fork-choice-fixes-6 branch from 65f93b3 to 740a8dc Compare July 30, 2020 13:41
@@ -273,7 +274,7 @@ proc init*(T: type BeaconNode,
onBeaconBlock(res, signedBlock)
)

await res.addLocalValidators()
res.addLocalValidators()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this might create race conditions with other parts of the code perhaps if the local validators aren't yet there?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, the code is more synchronous now without the await because addlocalvalidators is no longer asynchronous (notably, because it no longer has to schedule anything on the async loop)

for e in bs.blck.epochsInfo:
if e.epoch == epoch:
return e
if bs.slot == epoch.compute_start_slot_at_epoch:
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't blocks skip slots here, including the first slot of an epoch?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's why BlockSlot.parent moves slot-by-slot instead of block by block :)

@tersec
Copy link
Contributor

tersec commented Jul 30, 2020

TRC 2020-07-30 15:09:01+00:00 Updating checkpoint                        topics="fork_choice" tid=6956 file=fork_choice.nim:222 current_slot=2 best=ec48235b:0 current=ec48235b:0 updateAt=None[Epoch]
D:\a\1\s\tests\testutil.nim(80) test_attestation_pool
D:\a\1\s\tests\test_attestation_pool.nim(220) runSuite
D:\a\1\s\beacon_chain\attestation_pool.nim(345) selectHead
D:\a\1\s\beacon_chain\fork_choice\fork_choice.nim(337) find_head
D:\a\1\s\beacon_chain\fork_choice\fork_choice.nim(300) find_head
D:\a\1\s\beacon_chain\fork_choice\fork_choice.nim(414) compute_deltas
D:\a\1\s\vendor\nimbus-build-system\vendor\Nim\lib\system\fatal.nim(49) sysFatal

    Unhandled exception: value out of range: 32000000000 notin -2147483648 .. 2147483647 [RangeError]

On 32-bit Azure

@tersec
Copy link
Contributor

tersec commented Jul 30, 2020

But regardless, it seemed not to notice/care, and moved on to the next test:

D:\a\1\s\vendor\nimbus-build-system\vendor\Nim\lib\system\fatal.nim(49) sysFatal

    Unhandled exception: value out of range: 32000000000 notin -2147483648 .. 2147483647 [RangeError]
[NimScript] exec: nim c --out:./build/test_fixture_ssz_generic_types -r -d:chronicles_log_level=TRACE --verbosity:0 --hints:off -d:chronicles_log_level=TRACE -d:debug -d:disable_libbacktrace tests/official/test_fixture_ssz_generic_types.nim 

[Suite] Official - SSZ generic types
[OK] Testing basic_vector inputs - valid - skipping Vector[uint128, N] and Vector[uint256, N]
Unsupported **size** in type/size combination: array[0,bool]
Unsupported **size** in type/size combination: array[0,uint16]

...

10 longest individual test durations
------------------------------------
  0.44s for Testing containers   inputs - valid - skipping BitsStruct
  0.26s for Testing basic_vector inputs - invalid - skipping Vector[uint128, N] and Vector[uint256, N]
  0.16s for Testing bitlist      inputs - valid
  0.12s for Testing basic_vector inputs - valid - skipping Vector[uint128, N] and Vector[uint256, N]
  0.06s for Testing uints        inputs - valid - skipping uint128 and uint256
  0.03s for Testing bitvector    inputs - valid
  0.01s for Testing bitvector    inputs - invalid
  0.01s for Testing containers   inputs - invalid - skipping BitsStruct
  0.01s for Testing bitlist      inputs - invalid
  0.00s for Testing uints        inputs - invalid - skipping uint128 and uint256
  0.00s for Testing boolean      inputs - valid
[NimScript] exec: nim c --out:./build/test_fixture_ssz_consensus_objects -r -d:chronicles_log_level=TRACE -d:const_preset=mainnet --verbosity:0 --hints:off -d:chronicles_log_level=TRACE -d:debug -d:disable_libbacktrace tests/official/test_fixture_ssz_consensus_objects.nim 

[Suite] Official - SSZ consensus objects  [Preset: mainnet]
[OK]   Testing    AggregateAndProof
[OK]   Testing    Attestation
[OK]   Testing    AttestationData

And it's only on 32-bit Azure, and presumably other 32-bit platforms if CI ran them.

It's not new from this PR, e..g, #1387 shows the same pattern. 32-bit Azure "fails", but doesn't, and other platforms don't have that issue.

@arnetheduck arnetheduck merged commit c5fecd4 into devel Jul 30, 2020
@arnetheduck arnetheduck deleted the fork-choice-fixes-6 branch July 30, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants