Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Ensure that we fetch another collation if the first collation was invalid #3362

Merged
merged 2 commits into from
Jun 28, 2021

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jun 24, 2021

No description provided.

@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 24, 2021
target: LOG_TARGET,
?relay_parent,
candidate = ?candidate_receipt.hash(),
"Trying to insert a pending candidate failed, because there is already one!",
Copy link
Member

Choose a reason for hiding this comment

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

this should be unreachable, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be unreachable, yes. I just added it to have some error if we fucked it up.

Comment on lines 1076 to 1077
collations.status.back_to_waiting();
collations.get_next_collation_to_fetch()
Copy link
Member

Choose a reason for hiding this comment

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

these function are only called together, maybe it makes sense to merge them?

Comment on lines +1206 to +1207
if let Some(collations) = state.collations_per_relay_parent.get_mut(&relay_parent) {
collations.status = CollationStatus::WaitingOnValidation;
Copy link
Member

@ordian ordian Jun 24, 2021

Choose a reason for hiding this comment

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

how can it be None?
can the status be Seconded?
I feel like the state transition function could use more type-safety and checked assumptions/invariants built-in

Copy link
Member Author

Choose a reason for hiding this comment

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

It can not be none, but I also don't want to use an expect here.

I feel like the state transition function could use more type-safety and checked assumptions/invariants built-in

Not sure what you mean here.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like the state transition function could use more type-safety and checked assumptions/invariants built-in

Not sure what you mean here.

I don't have a suggestion how to refactor it yet, so nevermind.

Comment on lines +1206 to +1207
if let Some(collations) = state.collations_per_relay_parent.get_mut(&relay_parent) {
collations.status = CollationStatus::WaitingOnValidation;
Copy link
Member

Choose a reason for hiding this comment

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

I feel like the state transition function could use more type-safety and checked assumptions/invariants built-in

Not sure what you mean here.

I don't have a suggestion how to refactor it yet, so nevermind.

@ordian ordian merged commit 2b855f3 into master Jun 28, 2021
@ordian ordian deleted the bkchr-collation-fetch-on-invalid branch June 28, 2021 12:28
ordian added a commit that referenced this pull request Jul 2, 2021
* master: (21 commits)
  cleanup stream polls (#3397)
  Staking Miner (#3141)
  Companion for Substrate#8953 (#3140)
  Bump version, specs & substrate in prep for v0.9.8 (#3387)
  Fix busy loops. (#3392)
  Minor refactor (#3386)
  add simnet tests (#3381)
  BEEFY: adjust gossip (#3372)
  Companion for #9193 (#3376)
  Companion for Decouple Staking and Election - Part 3: Signed Phase (#2793)
  Ensure that we fetch another collation if the first collation was invalid (#3362)
  Only send one collation per relay parent at a time to validators (#3360)
  disable approval-checking-grandpa on dev chain (#3364)
  Use associated constant for max (#3375)
  Use wasm-builder from git (#3354)
  Squashed 'bridges/' changes from b2099c5..23dda62 (#3369)
  Bump versions & spec_versions (#3368)
  Don't allow bids for a ParaId where there is an overlapping lease period (#3361)
  Companion for upgrade of transaction-payment to pallet macro (#3267)
  Do not allow any crowdloan contributions during the VRF period (#3346)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants