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

collect included disputes from on-chain #3924

Merged
55 commits merged into from
Oct 6, 2021
Merged

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Sep 24, 2021

TODO:

  • hookup all runtimes to (roughly) runtime_impl::imported_disputes()-fns.
  • Assure graceful handling of imported_disputes on the node side, so older runtime versions that do not provide the call, do not take down the node.
  • add runtime tests
  • refactor
  • 🚲 🏠

@drahnr drahnr added A3-in_progress Pull request is in progress. No review needed at this stage. 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 Sep 24, 2021
@drahnr drahnr self-assigned this Sep 24, 2021
@drahnr drahnr force-pushed the bernhard-collect-on-chain-disputes branch 3 times, most recently from 803ec4c to 6d716d7 Compare September 28, 2021 16:23
@drahnr drahnr force-pushed the bernhard-collect-on-chain-disputes branch from 6d716d7 to b8cf3cd Compare September 28, 2021 17:03
@drahnr drahnr force-pushed the bernhard-collect-on-chain-disputes branch from b8cf3cd to 1b6a837 Compare September 28, 2021 17:04
primitives/src/v1/mod.rs Outdated Show resolved Hide resolved
@drahnr drahnr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. label Sep 29, 2021
@drahnr drahnr added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 4, 2021
primitives/src/v1/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

Looks good. Only needs implementers guide changes.

drahnr and others added 3 commits October 4, 2021 22:40
Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
return Ok(())
},
Ok(Err(e)) => {
tracing::debug!(
Copy link
Member

Choose a reason for hiding this comment

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

As this probably can come from a runtime upgrade, it should probably be warn?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it should be debug IMO otherwise the logs will constantly be filled with warn! until we upgrade the runtime to include this.

Copy link
Member

Choose a reason for hiding this comment

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

Fair, but should we make it warn once the runtime got upgraded? Follow up issue?

Copy link
Member

Choose a reason for hiding this comment

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

.validators
.get(validator_index.0 as usize)
.or_else(|| {
tracing::error!(
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@eskimor
Copy link
Member

eskimor commented Oct 6, 2021

Looks pretty good!

@@ -41,6 +42,7 @@ const SESSION_INFO_CACHE_SIZE: usize = 64 * 1024;
const DMQ_CONTENTS_CACHE_SIZE: usize = 64 * 1024;
const INBOUND_HRMP_CHANNELS_CACHE_SIZE: usize = 64 * 1024;
const CURRENT_BABE_EPOCH_CACHE_SIZE: usize = 64 * 1024;
const ON_CHAIN_VOTES_CACHE_SIZE: usize = 3 * 1024;
Copy link
Member

Choose a reason for hiding this comment

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

any reason for choosing 3KiB for cache size? the lowest we have is 64KiB, but it's not really backed by data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason. We simply won't use it that much, so this could be an optimization to remove it.

@drahnr
Copy link
Contributor Author

drahnr commented Oct 6, 2021

bot merge

@ghost
Copy link

ghost commented Oct 6, 2021

Trying merge.

@ghost ghost merged commit b74335a into master Oct 6, 2021
@ghost ghost deleted the bernhard-collect-on-chain-disputes branch October 6, 2021 19:16
This pull request was closed.
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants