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

Move is_data_available check to fork-choice on_block #3185

Merged
merged 6 commits into from
Jan 6, 2023

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jan 3, 2023

Address #3170

Note: need to check again the confidence level of this change.

@hwwhww hwwhww added scope:fork-choice Deneb was called: eip-4844 labels Jan 3, 2023
@hwwhww hwwhww requested review from djrtwo and asn-d6 January 3, 2023 16:25
@dapplion dapplion mentioned this pull request Jan 4, 2023
10 tasks
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I left some relatively minor comments on file/definition organization

Also, the fact that we have no tests change here is a bit unnerving -- seeming we don't have any tests for is_data_available() within fork-choice or state transition.

It might be good to upgtrade the fork-choice test format to include if a block is available. (and if not, be able to subsequently become available) but I'm not sure if we should do so here

specs/eip4844/fork-choice.md Outdated Show resolved Hide resolved
specs/eip4844/fork-choice.md Outdated Show resolved Hide resolved
specs/eip4844/fork-choice.md Outdated Show resolved Hide resolved
specs/eip4844/fork-choice.md Show resolved Hide resolved
specs/eip4844/fork-choice.md Outdated Show resolved Hide resolved
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
@djrtwo djrtwo mentioned this pull request Jan 6, 2023
5 tasks
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

excellent!

@hwwhww
Copy link
Contributor Author

hwwhww commented Jan 6, 2023

It might be good to upgtrade the fork-choice test format to include if a block is available. (and if not, be able to subsequently become available) but I'm not sure if we should do so here

We could do something like the way we hacked get_pow_block in on_merge_block tests:

def with_pow_block_patch(spec, blocks, func):
def get_pow_block(hash: spec.Bytes32) -> spec.PowBlock:
for block in blocks:
if block.block_hash == hash:
return block
raise BlockNotFoundException()
get_pow_block_backup = spec.get_pow_block
spec.get_pow_block = get_pow_block
class AtomicBoolean():
value = False
is_called = AtomicBoolean()
def wrap(flag: AtomicBoolean):
yield from func()
flag.value = True
try:
yield from wrap(is_called)
finally:
spec.get_pow_block = get_pow_block_backup
assert is_called.value

I'll open a ticket, but probably add it in the next next release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844 scope:fork-choice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants