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

Explicitly Handling ProvisionableData Cases #6757

Merged
merged 14 commits into from
Feb 24, 2023
Merged

Explicitly Handling ProvisionableData Cases #6757

merged 14 commits into from
Feb 24, 2023

Conversation

BradleyOlson64
Copy link
Contributor

ProvisionerMessage::ProvisionableData can pass along two data variants for which we want to do nothing. Previously these were ignored using a wildcard, which is bad form.

This PR explicitely handles those cases, explaining why we don't need to address them during the backing phase. A couple lines of logging and implementer's guide changes accompany.

@BradleyOlson64 BradleyOlson64 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 Feb 22, 2023
@BradleyOlson64
Copy link
Contributor Author

BradleyOlson64 commented Feb 22, 2023

Cargo.lock is lagging behind Cargo.toml on master. Not sure if this is something we care about, but I figure it's at least worth asking about and attempting to rectify here. Seems like Cargo.lock wouldn't be tracked if we didn't want it updated.

// backing stage, can't be guaranteed to conclude. Also candidates which haven't
// been made available don't pose a security risk as they can not be included,
// approved, or finalized. Thus we wait to punish misbehavior until a candidate
// reaches the approval checking stage.
Copy link
Member

Choose a reason for hiding this comment

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

The main point here is, we cannot dispute at this stage. What the backer could do is record the fact that the candidate was invalid and dispute it once it gets included. This would add to security a bit, but not too much as backers are known ahead of time and we assume they can be dosed. Anyhow if we decide the complexity is worth it, we can do it at some point: #3232 <- Referencing this ticket here might make sense as well.

@eskimor eskimor changed the title srlabs_findings #102: Explicitly Handling ProvisionableData Cases Explicitly Handling ProvisionableData Cases Feb 22, 2023
@BradleyOlson64 BradleyOlson64 self-assigned this Feb 22, 2023
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Great, thanks for clarifying things! I don't see a reason to change Cargo.lock in this PR though, as it's an unrelated change. Can you undo that?

@@ -324,7 +324,23 @@ fn note_provisionable_data(
.with_para_id(backed_candidate.descriptor().para_id);
per_relay_parent.backed_candidates.push(backed_candidate)
},
_ => {},
// We choose to do nothing with misbehavior at this stage
Copy link
Member

Choose a reason for hiding this comment

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

We should argue why not here as well. The misbehavior is self-contained and the validator could get punished. The reason we don't is, because we already have mitigation on the protocol level (reputation changes) and for the time being those seem to be enough.

@BradleyOlson64
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for 290b63b

@BradleyOlson64
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for 290b63b

@BradleyOlson64 BradleyOlson64 removed the request for review from tdimitrov February 23, 2023 20:00
@BradleyOlson64
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for 290b63b

@eskimor eskimor merged commit d56dcfa into master Feb 24, 2023
@eskimor eskimor deleted the brad-issue-102 branch February 24, 2023 20:54
ordian added a commit that referenced this pull request Mar 16, 2023
* master: (98 commits)
  Ensure max_weight is assigned properly in AllowTopPaidExecutionFrom (#6787)
  Explicitly Handling ProvisionableData Cases (#6757)
  Companion for Substrate#12520 (#6730)
  Revert back to bare metal runners for weights generation (#6762)
  Improve XCM fuzzer (#6190)
  Corrected weight trader comment (#6752)
  clean up executed migrations (#6763)
  Remove state migration from westend runtime. (#6737)
  polkadot companion #12608 (Pools claim permissions) (#6753)
  Add Turboflakes bootnodes to Polkadot, Kusama and Westend (#6628)
  Companion for substrate#13284 (#6653)
  Companion for Substrate #13410: Introduce EnsureOrigin to democracy.propose (#6750)
  `BlockId` removal: `BlockBuilderProvider::new_block_at` (#6734)
  Companion PR for PR#13119 (#6683)
  Companion for Substrate#13411: frame/beefy: prune entries in set id session mapping (#6743)
  `BlockId` removal: refactor of runtime API (#6721)
  Fix auction bench (#6747)
  Use PVF code paired with executor params wherever possible (#6742)
  Retire `OldV1SessionInfo` (#6744)
  Companion for substrate #13121 - BEEFY Equivocations support (#6593)
  ...
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants