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

Companion to "Updating scale to v3" #4958

Merged
merged 11 commits into from
Feb 25, 2022
Merged

Companion to "Updating scale to v3" #4958

merged 11 commits into from
Feb 25, 2022

Conversation

wigy-opensource-developer
Copy link
Contributor

@wigy-opensource-developer wigy-opensource-developer commented Feb 21, 2022

Companion to paritytech/substrate#10825

Note that bitvec:1.0.0 has breaking changes that are not trivial to fix. Other crates with breaking changes are way simpler to fix.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Feb 21, 2022
@ordian ordian added B1-releasenotes C1-low PR touches the given topic and has a low impact on builders. labels Feb 21, 2022
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Marking as release-notes because of paritytech/substrate#10825 (comment).

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

For this one (and the other bumps to v3), I want to ensure that none of the existing code relied on the non-advancement of the cursor during decoding, so it's important to check for the usages of decode, decode_all, decode_with_depth_limit, decode_all_with_depth_limit and especially decode_and_advance_with_depth_limit, the last of which can just be changed to decode_with_depth_limit.

@ordian ordian added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. label Feb 24, 2022
@wigy-opensource-developer
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Checks failed for 2ac0e0b

@ordian
Copy link
Member

ordian commented Feb 24, 2022

I'm tempted to merge this manually, but the failing dispute-valid test is concerning: https://gitlab.parity.io/parity/polkadot/-/jobs/1402257

@wigy-opensource-developer
Copy link
Contributor Author

Yeah it was a little late in the chain, but at least this integration test caught a serialization format regression. Fix is on the way paritytech/scale-info#143

@ordian
Copy link
Member

ordian commented Feb 24, 2022

Yeah it was a little late in the chain, but at least this integration test caught a serialization format regression. Fix is on the way paritytech/scale-info#143

I think we're talking about different tests. The one that caught the regression was smoke-test.

@@ -18,7 +18,7 @@ polkadot-primitives = { path = "../../primitives" }
polkadot-overseer-gen = { path = "./overseer-gen" }
tracing = "0.1.31"
lru = "0.7"
parity-util-mem = { version = ">= 0.10.1", default-features = false }
parity-util-mem = { version = "0.11.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is narrowing the allowed versions from both directions.

Copy link
Member

Choose a reason for hiding this comment

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

parity-util-mem doesn't allow duplicates on the link level, so it doesn't change anything

*a |= state.validators_against.iter().by_val();
a
};
let all_participants = state.validators_for.clone() | state.validators_against.clone();
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 not available any more in bitvec:1.0.0, but I believe the replacement I wrote here achieves the same result.

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.rs/bitvec/1.0.0/bitvec/vec/struct.BitVec.html#impl-BitOrAssign%3C%26%27_%20BitVec%3CT%2C%20O%3E%3E

Please keep the code as close as possible to the previous version. Refactoring can be done in a separate PR.
I don't know for sure what's causing the failing dispute test, but we need to exclude this factor.

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 not supported for .iter().by_vals() anymore. |= is implemented for BitSlice and BitVec right-hand-sides. BitVec borrowed as a BitSlice in the implementation. | is implemented using |=, it is just more verbose using it. Also, all the unit tests testing this and the following change are passing and cover these pieces of code quite well.

test disputes::tests::test_contains_duplicates_in_sorted_iter ... ok
test disputes::tests::test_has_supermajority_against ... ok
test disputes::tests::test_decrement_spam ... ok
test disputes::tests::test_dispute_state_flag_from_state ... ok
test disputes::tests::assure_no_duplicate_statements_sets_are_fine ... ok
test disputes::tests::test_import_new_participant_spam_inc ... ok
test disputes::tests::test_import_slash_against ... ok
test disputes::tests::test_import_prev_participant_spam_dec_confirmed_slash_for ... ok
test disputes::tests::test_check_signature ... ok

I am kind of puzzled what causes breakage in the dispute integration tests, but it seems to be more subtle than these changes themselves.

// all participants, which are not new participants
let prev_participants = (self.state.validators_for.clone() |
self.state.validators_against.clone()) &
!self.new_participants.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this was trickier, but I still think self.new_participants has the same number of bits that self.state.validators_for and self.state.validators_against has (see let new_participants = bitvec::bitvec![u8, BitOrderLsb0; 0; state.validators_for.len()] above).

There is also a chance that the whole algorithm gets simpler/faster if merged with the next .iter_ones().map(...).collect() transformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could replace self.state.validators_against.clone() with a borrow here, otherwise it matches the previous implementation.

@ordian
Copy link
Member

ordian commented Feb 25, 2022

ok, looking at the last commits: https://github.com/paritytech/polkadot/commits/master
it seems like this test failed starting from

In any case, I think this is unrelated to the changes here.

@ordian ordian merged commit 1a5d463 into master Feb 25, 2022
@ordian ordian deleted the wigy-update-scale3 branch February 25, 2022 12:07
svyatonik added a commit to paritytech/parity-bridges-common that referenced this pull request Mar 15, 2022
svyatonik added a commit to paritytech/parity-bridges-common that referenced this pull request Mar 15, 2022
* cumulus: 4e95228
polkadot: 975e780ae0d988dc033f400ba822d14b326ee5b9
substrate: 89fcb3e4f62d221d4e161a437768e77d6265889e

* fix refs

* sync changes from paritytech/polkadot#3828

* sync changes from paritytech/polkadot#4387

* sync changes from paritytech/polkadot#3940

* sync with changes from paritytech/polkadot#4493

* sync with changes from paritytech/polkadot#4958

* sync with changes from paritytech/polkadot#3889

* sync with changes from paritytech/polkadot#5033

* sync with changes from paritytech/polkadot#5065

* compilation fixes

* fixed prometheus endpoint startup (it now requires to be spawned within tokio context)
svyatonik added a commit to paritytech/parity-bridges-common that referenced this pull request Mar 21, 2022
* Fix mandatory headers scanning in on-demand relay (#1306)

* more logging in on-demand headerss

* remove `maximal_headers_difference` concept from on-demand-relay

* another leftover from previous on-demand version

* removed extra log

* fmt

* increase relay balance guard limits for Polkadot<>Kusama bridge (#1308)

* fix benchmarks before using it in Polkadot/Kusama/Rococo runtimes (#1309)

* Endow relayer account at target chain in message benchmarks (#1310)

* endow relayer account at target chain in message benchmarks

* pick another line

* expose fee multiplier metrics in messages relay (#1312)

* Fix issues from cargo deny (#1311)

* update libp2p-core (RUSTSEC-2022-0009)

* update thread_local (RUSTSEC-2022-0006)

* time 0.2 -> time 0.3

* ignore RUSTSEC-2021-0130

* proper migration to time 0.3

* fix clippy?

* Revert "fix clippy?"

This reverts commit 53bc289.

* Add some tests to check integrity of chain constants + bridge configuration (#1316)

* add some tests to check integrity of chain constants + bridge configuration

* try to use named parameters where possible

* Encode and estimate Rococo/Wococo/Kusama/Polkadot messages (#1322)

* encode and estimate Rococo/Wococo/Kusama/Polkadot messages

* allow send-message for non-bundled chains

* weight -> dispatch-weight

* fmt

* fix spelling

* impl Decode for SignedExtensions (otherwise transaction resubmitter panicks) (#1325)

* use mortal transactions in transaction resubmitter (#1326)

* Using-same-fork metric for finality and complex relay (#1327)

* using_same_fork metric in finality relay

* support `using_different_forks` in messages relay

* added dashboards and alerts

* lockfile

* fix typo in alert expression (#1329)

* removed extra *_RUNTIME_VERSION consts from relay code (#1330)

* Reinitialize bridge relay subcommand (#1331)

* reinitialize bridge subcommand

* PolkadotToKusama in reinit-bridge

* fix clippy issues (#1332)

* Revert "Revert "override conversion rate in estimate-message-fee RPC (#1189)" (#1275)" (#1333)

This reverts commit ed7def6.

* Override conversion rate when computing message fee (#1261)

* override conversion rate when message is sent

* spelling + fmt

* add --conversion-rate-override cli option

* try to read conversion rate from cmd output

* fix output

* fmt

* fi typo in generator script (#1334)

* override conversion rate in tokens swap generator (#1335)

* fix conversion rate override in token swap (#1336)

* synchronize relay cli changes and token swap generator script (#1337)

* fixed mess with conversion rates (#1338)

* fix generator scripts to be consistent with updatedrelay output (#1339)

* Increase rate from metric when estimating fee (#1340)

* ignore errors when dumping logs and container is missing

* fixed typo

* print correct payload length

* increase conversion rate a bit when estimating fee (to avoid message rejects when rate update tx is active)

* fmt

* fix conversion rate metric in dashboards (#1341)

* get rid of '[No Data] Messages from Millau to Rialto are not being delivered' warnings (#1342)

* use DecodeLimit when decoding incoming calls (#1344)

* update regex to 1.5.5 (#1345)

* edition = "2021" (#1346)

* Mortal conversion rate updater transactions (#1257)

* merge all similar update_conversion_rate functions

* stall timeout in conversion rate update loop

* fmt

* fix

* added alerts for relay balances (#1347)

* replace From<>InboundLaneApi with direct storage reads (#1348)

* added no_stack_overflow_when_decoding_nested_call_during_dispatch test (#1349)

* ignore more "increase" alerts that are sometimes signalling NoData at startup (#1351)

* Support dedicated lanes for pallets (#962)

* pass call origin to the message verifier

* is_outbound_lane_enabled -> is_message_accepted

* trait SenderOrigin

* only accept messages from token swap pallet to token swap lane

* tests for edge cases of pay_delivery_and_dispatch_fee

* fixed origin verification

* fmt

* fix benchmarks compilation

* fix TODO with None account and non-zero message fee (already covered by tests)

* revert cargo fmt changes temporarily

* Update Substrate/Polkadot/Cumulus references (#1353)

* cumulus: 4e95228
polkadot: 975e780ae0d988dc033f400ba822d14b326ee5b9
substrate: 89fcb3e4f62d221d4e161a437768e77d6265889e

* fix refs

* sync changes from paritytech/polkadot#3828

* sync changes from paritytech/polkadot#4387

* sync changes from paritytech/polkadot#3940

* sync with changes from paritytech/polkadot#4493

* sync with changes from paritytech/polkadot#4958

* sync with changes from paritytech/polkadot#3889

* sync with changes from paritytech/polkadot#5033

* sync with changes from paritytech/polkadot#5065

* compilation fixes

* fixed prometheus endpoint startup (it now requires to be spawned within tokio context)

* update chain versions (#1358)

* update parity-scale-codec to 3.1.2 (#1359)

* fix parse_transaction on Rialto+Millau (#1360)
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. 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