Allow multiple bids compatible with the head view (alpha.13) - #9763
Conversation
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
mergify
Bot
added
waiting-on-author
9e7ae50 to
89ef3ab
Compare
bc145f5 to
01bea64
Compare
01bea64 to
6227fc2
Compare
6227fc2 to
bc6ef4c
Compare
bc6ef4c to
1467308
Compare
1467308 to
8576c50
Compare
8576c50 to
8177335
Compare
8177335 to
5d66b2f
Compare
| // TODO into consideration maybe should persist parent block hash and gas limit in fork choice? | ||
| if let Ok(parent_bid) = head_state.latest_execution_payload_bid() | ||
| && !is_gas_limit_target_compatible( | ||
| parent_bid.gas_limit, |
There was a problem hiding this comment.
this would make us reject the bid if its a different gas limit than the head right? so this still needs to be addressed afaiu
There was a problem hiding this comment.
i was thinking about this a bit today. i think the two "best" options are an added field on proto array to keep track of gas limits, or some cache. im leaning towards adding a field on proto array, wdyt? I think my plan would be to add it as a separate stacked PR
There was a problem hiding this comment.
I think we can resolve this after we merge this stack
|
|
||
| let builder_index = bid.builder_index; | ||
|
|
||
| let is_active_builder = head_state |
There was a problem hiding this comment.
Codex flagged this as an issue at fork boundary because of the other changes which I think is accurate.
The head state here is the canonical head state which at the fork boundary is not advanced to gloas. So is_active_builder would fail here until we get the first consensus block of gloas and update canonical head accordingly.
Fixing this might require us loading the advanced state instead which isn't ideal. I'm inclined to keep this as is, but thought it was worth flagging
There was a problem hiding this comment.
ah nice catch. yeah if we dont handle this we wont be able to accept a bid at the fork boundary. not the worst thing ever, but the fix is straightforward and we can deprecate it after mainnet forks to gloas
chong-he
left a comment
chong-he
left a comment
There was a problem hiding this comment.
The main changes is_bid_compatible_with_head is following the spec from what I understand, but it has no tests to cover, should we add some tests to cover different scenarios?
| pub struct BidParent { | ||
| pub parent_block_hash: ExecutionBlockHash, | ||
| pub parent_block_root: Hash256, | ||
| } |
There was a problem hiding this comment.
Maybe it's just me? I would think that ParentBid is more intuitive than BidParent in the naming of this struct
There was a problem hiding this comment.
i chose BidParent because it is the bids parent. I think ParentBid could be confusing because it sounds like its pointing towards the "parent bid"
044d633 to
778e228
Compare
0640b21 to
700835f
Compare
Merge Queue Status
This pull request spent 11 minutes 2 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #9763 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
pawanjay176
added
ready-for-review
Merge Queue Status
This pull request spent 42 minutes 45 seconds in the queue, including 27 minutes 47 seconds running CI. Required conditions to merge
|
3 participants
Issue Addressed
ethereum/consensus-specs#5497
Depends-On: #9761