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

Make unbounded channels size warning exact (part 1) #13490

Merged
merged 6 commits into from
Mar 1, 2023

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Feb 28, 2023

Obsoletes #13117. I am also going to replace futures-channel with async-channel in client/utils/mpsc, but this will break the API of TracingUnboundedSender (async-channel does not implement Sink trait), so decided to start with smaller changes in out_events.rs

polkadot companion: paritytech/polkadot#6800

CC @nazar-pc

@dmitry-markin dmitry-markin 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Feb 28, 2023
client/network/src/service/out_events.rs Outdated Show resolved Hide resolved
Comment on lines 186 to 187
"The number of unprocessed events in channel `{}` exceeded {}.\n\
The channel was created at:\n{:?}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.... shouldn't the backtrace be printed out with {} instead of {:?}?

Maybe also print out the backtrace from which this send is being made? I can imagine that could also be useful in some cases. It's a one time warning that should only trigger in exceptional cases, so I think we can afford it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

backtrace::Backtrace doesn't implement std::fmt::Display

Copy link
Contributor

Choose a reason for hiding this comment

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

backtrace::Backtrace doesn't implement std::fmt::Display

I see. The one from std does though, if we'd switch to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The std backtrace is resolved at the moment it's captured, so it was proposed by @bkchr to use backtrace::Backtrace, which can be resolved only if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The std backtrace is resolved at the moment it's captured

Is it? I just checked before proposing it, and it doesn't seem to be? Unless I'm reading it wrong.

https://github.com/rust-lang/rust/blob/72067c77bdc1e8e339b9ed378a2c0ca0a9367c4d/library/std/src/backtrace.rs#L350

Here's where it's resolved:

https://github.com/rust-lang/rust/blob/72067c77bdc1e8e339b9ed378a2c0ca0a9367c4d/library/std/src/backtrace.rs#L443

And that's called only when frames are called (so only when the backtrace is actually accessed, which is from the Display impl below):

https://github.com/rust-lang/rust/blob/72067c77bdc1e8e339b9ed378a2c0ca0a9367c4d/library/std/src/backtrace.rs#L379

Am I reading this wrong? (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe also print out the backtrace from which this send is being made? I can imagine that could also be useful in some cases. It's a one time warning that should only trigger in exceptional cases, so I think we can afford it.

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it? I just checked before proposing it, and it doesn't seem to be? Unless I'm reading it wrong.

Didn't check myself 🙈 Will fix in a moment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@koute
Copy link
Contributor

koute commented Feb 28, 2023

Also, std::backtrace::Backtrace is stable since 1.65, so we can probably switch to it instead of using the backtrace crate.

Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

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

You can also remove the backtrace dependency from Cargo.toml. (:

@dmitry-markin
Copy link
Contributor Author

You can also remove the backtrace dependency from Cargo.toml. (:

Just lost Cargo.toml edit in a pile of unrelated changes

@dmitry-markin
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 5cd4847 into master Mar 1, 2023
@paritytech-processbot paritytech-processbot bot deleted the dm-async-channel branch March 1, 2023 09:54
gpestana pushed a commit that referenced this pull request Mar 1, 2023
* Replace `futures-channel` with `async-channel` in `out_events`

* Apply suggestions from code review

Co-authored-by: Koute <koute@users.noreply.github.com>

* Also print the backtrace of `send()` call

* Switch from `backtrace` crate to `std::backtrace`

* Remove outdated `backtrace` dependency

* Remove `backtrace` from `Cargo.lock`

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* Replace `futures-channel` with `async-channel` in `out_events`

* Apply suggestions from code review

Co-authored-by: Koute <koute@users.noreply.github.com>

* Also print the backtrace of `send()` call

* Switch from `backtrace` crate to `std::backtrace`

* Remove outdated `backtrace` dependency

* Remove `backtrace` from `Cargo.lock`

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Replace `futures-channel` with `async-channel` in `out_events`

* Apply suggestions from code review

Co-authored-by: Koute <koute@users.noreply.github.com>

* Also print the backtrace of `send()` call

* Switch from `backtrace` crate to `std::backtrace`

* Remove outdated `backtrace` dependency

* Remove `backtrace` from `Cargo.lock`

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
paritytech-processbot bot pushed a commit that referenced this pull request Aug 10, 2023
* Implements dynamic nominations per nominator

* Adds SnapshotBounds and ElectionSizeTracker

* Changes the ElectionDataProvider interface to receive ElectionBounds as input

* Implements get_npos_voters with ElectionBounds

* Implements get_npos_targets with ElectionBounds

* Adds comments

* tests

* Truncates nomninations that exceed nominations quota; Old tests passing

* Uses DataProviderBounds and ElectionBounds (to continue)

* Finishes conversions - tests passing

* Refactor staking in babe mocks

* Replaces MaxElectableTargets and MaxElectingVoters with ElectionBounds; Adds more tests

* Fixes nits; node compiling

* bechmarks

* removes nomination_quota extrinsic to request the nomination quota

* Lazy quota check, ie. at nominate time only

* remove non-working test (for now)

* tests lazy nominations quota when quota is lower than current number of nominated targets

* Adds runtime API and custom RPC call for clients to query the nominations quota for a given balance

* removes old rpc

* Cosmetic touches

* All mocks working

* Fixes benchmarking mocks

* nits

* more tests

* renames trait methods

* nit

* ".git/.scripts/commands/fmt/fmt.sh"

* Fix V2 PoV benchmarking (#13485)

* Bump default 'additional_trie_layers' to two

The default here only works for extremely small runtimes, which have
no more than 16 storage prefices. This is changed to a "sane" default
of 2, which is save for runtimes with up to 4096 storage prefices (eg StorageValue).

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update tests and test weights

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix PoV weights

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_balances

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_message_queue

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_glutton

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_glutton

* Fix sanity check

>0 would also do as a check, but let's try this.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>

* Move BEEFY code to consensus (#13484)

* Move beefy primitives to consensus dir
* Move beefy gadget to client consensus folder
* Rename beefy crates

* chore: move genesis block builder to chain-spec crate. (#13427)

* chore: move genesis block builder to block builder crate.

* add missing file

* chore: move genesis block builder to sc-chain-spec

* Update client/chain-spec/src/genesis.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* Update test-utils/runtime/src/genesismap.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* Update test-utils/runtime/client/src/lib.rs

* fix warnings

* fix warnings

---------

Co-authored-by: Bastian Köcher <git@kchr.de>

* Speed up storage iteration from within the runtime (#13479)

* Speed up storage iteration from within the runtime

* Move the cached iterator into an `Option`

* Use `RefCell` in no_std

* Simplify the code slightly

* Use `Option::replace`

* Update doc comment for `next_storage_key_slow`

* Make unbounded channels size warning exact (part 1) (#13490)

* Replace `futures-channel` with `async-channel` in `out_events`

* Apply suggestions from code review

Co-authored-by: Koute <koute@users.noreply.github.com>

* Also print the backtrace of `send()` call

* Switch from `backtrace` crate to `std::backtrace`

* Remove outdated `backtrace` dependency

* Remove `backtrace` from `Cargo.lock`

---------

Co-authored-by: Koute <koute@users.noreply.github.com>

* Removal of Prometheus alerting rules deployment in cloud-infra (#13499)

* sp-consensus: remove unused error variants (#13495)

* Expose `ChargedAmount` (#13488)

* Expose `ChargedAmount`

* Fix imports

* sc-consensus-beefy: fix metrics: use correct names (#13494)


Signed-off-by: acatangiu <adrian@parity.io>

* clippy fix

* removes NominationsQuotaExceeded event

* Update frame/staking/src/lib.rs

Co-authored-by: Ross Bulat <ross@parity.io>

* adds back the npos_max_iter

* remove duplicate imports added after merge

* fmt

* Adds comment in public struct; Refactors CountBound and SizeCount to struct

* addresses various pr comments

* PR comment reviews

* Fixes on-chain election bounds and related code

* EPM checks the size of the voter list returned by the data provider

* cosmetic changes

* updates e2e tests mock

* Adds more tests for size tracker and refactors code

* Adds back only_iterates_max_2_times_max_allowed_len test

* Refactor

* removes unecessary dependency

* empty commit -- restart all stuck CI jobs

* restarts ci jobs

* Renames ElectionBounds -> Bounds in benchmarking mocks et al

* updates mocks

* Update frame/election-provider-support/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Update frame/staking/src/pallet/impls.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Update frame/election-provider-support/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Update frame/staking/src/tests.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* more checks in api_nominations_quota in tests

* Improves docs

* fixes e2e tests

* Uses size_hint rather than mem::size_of in size tracker; Refactor size tracker to own module

* nits from reviews

* Refactors bounds to own module; improves docs

* More tests and docs

* fixes docs

* Fixes benchmarks

* Fixes rust docs

* fixes bags-list remote-ext-tests

* Simplify bound checks in create_snapshot_external

* Adds target size check in get_npos_targets

* ".git/.scripts/commands/fmt/fmt.sh"

* restart ci

* rust doc fixes and cosmetic nits

* rollback upgrade on parity-scale-codec version (unecessary)

* reset cargo lock, no need to update it

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: acatangiu <adrian@parity.io>
Co-authored-by: command-bot <>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Davide Galassi <davxy@datawok.net>
Co-authored-by: yjh <yjh465402634@gmail.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Koute <koute@users.noreply.github.com>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Anthony Lazam <lazam@users.noreply.github.com>
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: Piotr Mikołajczyk <piomiko41@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Ross Bulat <ross@parity.io>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
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
Status: done
Development

Successfully merging this pull request may close these issues.

3 participants