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

Fix wrong deposit amount in council voters. #2562

Merged
7 commits merged into from
Mar 17, 2021
Merged

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Mar 4, 2021

As pointed out correctly by @AurevoirXavier

In https://github.com/paritytech/polkadot/pull/1719/files#diff-e5e76e02c0d16e79c70b024cbe3c6ea56f3249382a0f987ba203c34fcb40ed66 we made a mistake and passed in an incorrect value (5 CENT) as the old voting bond to the migration, instead of the correct value, 5 DOLLARs.

Thus, currently all council voters have 50 milli-dots in deposit recorded for them, whereas it should be 5 DOTs.

Screenshot 2021-03-04 at 18 50 17

  • Do a cost analysis for the number of voters that we can support within 2 blocks, and increase bonds if needed.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 4, 2021
@kianenigma kianenigma added B0-silent Changes should not be mentioned in any release notes C7-high labels Mar 4, 2021
@kianenigma
Copy link
Contributor Author

Once released, I'll do another damage analysis and find the voter's who got affected by this bug (anyone who changed or removed their vote sine runtime 27) and find ways to refund them.

@kianenigma kianenigma added this to the v0.8.30 milestone Mar 15, 2021
runtime/polkadot/src/lib.rs Outdated Show resolved Hide resolved
runtime/polkadot/src/lib.rs Outdated Show resolved Hide resolved
runtime/polkadot/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

@kianenigma kianenigma added B7-runtimenoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Mar 17, 2021
@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 17, 2021

Waiting for commit status.

@kianenigma
Copy link
Contributor Author

bot merge abort

@kianenigma
Copy link
Contributor Author

bot abort

@kianenigma
Copy link
Contributor Author

bot stop

@kianenigma
Copy link
Contributor Author

bot please stop

@kianenigma
Copy link
Contributor Author

Well, I think I can no longer stop the bot. Not a big deal. I wanted to add a further test as well, but it is fine.

@kianenigma
Copy link
Contributor Author

bot help

@ghost
Copy link

ghost commented Mar 17, 2021

Head SHA changed; merge aborted.

@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 17, 2021

Waiting for commit status.

@ghost ghost merged commit 6d68c40 into master Mar 17, 2021
@ghost ghost deleted the kiz-fix-council-voters-deposit branch March 17, 2021 12:13
@xlc
Copy link
Contributor

xlc commented Mar 17, 2021

maybe you need to ask nicely. i.e. sudo bot stop 😆

ordian pushed a commit that referenced this pull request Mar 19, 2021
* Fix wrong deposit amount in council voters.

* Fix some build

* make it all compile.. so far.

* Fix

* break build

* Okay fix it again
ordian added a commit that referenced this pull request Mar 19, 2021
* master:
  Don't accept incoming connections for collators (#2644)
  Improve the logging (#2645)
  Update for the new substrate client API (#2570)
  integrate faster erasure code (#2608)
  Companion for #8372 (Replace 'Module' with 'Pallet' in construct_runtime macro) (#2629)
  Request based collation fetching (#2621)
  Companion for Substrate#8386 (#2634)
  Polkadot companion for Substrate PR #7640 (Store multiple Justifications per block) (#2358)
  yet another set of logging improvements (#2638)
  Reduce number of active leaves at startup (#2631)
  re benchmark  (#2630)
  Fix wrong deposit amount in council voters. (#2562)
  Add /data symlink to Docker containers (#2627)
  Companion for sub/8176 (#2622)
  Remove TODO from substrate#2986 (#2628)
  update ring to 0.16.20 (#2626)
  New slots/auctions architecture (#2294)
  add tracing when no assignment in candidate selection (#2623)
  Backing and collator protocol traces including para-id (#2620)
  more diagnostic logs for approval-voting (#2618)
ordian added a commit that referenced this pull request Mar 24, 2021
* ci: initial fuzzer job

* erasure-coding: update fuzzer Cargo.lock

* syntax fix

* try this first

* install honggfuzz deps

* try not

* try if else

* try SIGINT

* ignore hfuzz dirs

* ???

* bash is growing on me

* decouple builds from running

* fix a typo

* try copying dirs

* fix indentation

* try using absolute paths

* another try

* caching is not worth it

* remove outdated needs

* cleanup and add futher TODOs

* Update .github/workflows/honggfuzz.yml

* more diagnostic logs for approval-voting (#2618)

* Backing and collator protocol traces including para-id (#2620)

* improve backing/provisioner spans

* span for collation requests

* add para_id to unbacked candidate spans

* differentiate validation-construction and find-assignment in selection

* better find-assignment spans

* organize unbacked-candidate spans directly under job root

* Update node/core/provisioner/src/lib.rs

Co-authored-by: Andronik Ordian <[email protected]>

Co-authored-by: Andronik Ordian <[email protected]>

* add tracing when no assignment in candidate selection (#2623)

* New slots/auctions architecture (#2294)

* TODOs

* Add auctions.rs, comment on changes needed.

* Remove cruft from slots

* Remove more from auctions.rs

* More logic drafting in slots.

* More logic in slots.rs

* patch some errors

* more fixes

* last nit

* Cleanups in slots.rs

* Cleanups in slots.rs

* patches

* make build

* crowdloan to new api

* auction compile

* Use ParaId instead of FundIndex in Crowdloan (#2303)

* use paraid instead of fundindex

* Update crowdloan.rs

* check caller is manager

* Auction tests and fix build warnings.

* Configurable origin for initiating auctions

* Remove on_finalize

* #2303 (manual merge)

* Tests for Slots

* some registrar tests

* Apply suggestions from code review

Co-authored-by: Guillaume Thiolliere <[email protected]>

* Update runtime/common/src/slots.rs

Co-authored-by: Guillaume Thiolliere <[email protected]>

* Slots uses Registrar for CurrentChains

* swap works test

* on swap impl

* traitify parachain cleanup

* explicit lifecycle tracking for paras

* initial implementation of lifecycles and upgrades

* clean up a bit

* Update runtime/common/src/slots.rs

Co-authored-by: Guillaume Thiolliere <[email protected]>

* fix doc comment

* more rigid lifecycle checks

* include paras which are transitioning, and lifecycle query

* format guide

* update api

* update guide

* explicit outgoing state, fix genesis

* handle outgoing with transitioning paras

* Revert "explicit lifecycle tracking for paras"

This reverts commit 4177af7.

* remove lifecycle tracking from registrar

* do not include transitioning paras in identifier

* Update paras_registrar.rs

* final patches to registrar

* Fix test

* use noop in test

* clean up pending swap on deregistration

* finish registrar tests

* Update roadmap/implementers-guide/src/runtime/paras.md

* Update roadmap/implementers-guide/src/runtime/paras.md

* Update roadmap/implementers-guide/src/runtime/paras.md

* Apply suggestions from code review

* Use matches macro

* Correct terms

* Apply suggestions from code review

* Remove direct need for Slots and Registrar from Crowdloan

* Rejig things slightly

* actions queue

* Revert "actions queue"

This reverts commit b2e9011.

* Traitify Auction interface.

* Mockups and initial code for Crowdloan testing

* One test...

* collapse onboarding state

* fix some crowdloan tests

* one more

* start benchmarks for auctions

* benchmark bid

* fix more crowdloan tests

* onboard and begin retirement no longer exist

* Revert "onboard and begin retirement no longer exist"

This reverts commit 2e100fd.

* Simplify crowdloan and make it work.

* Fixes

* fix some

* finish merge fixes

* fix refund bug in auctions

* Add traits to Registrar for tests and benchmarks

* fix more auction benchmarks

* Fix TestAuctioneer

* finish crowdloan benchmarks

* start setting up full integration tests

* expand integration tests

* finish basic integration test

* add more integration tests

* begin slots benchmarks

* start paras registrar benchmarks

* fix merge

* fix tests

* clean up paras registrar

* remove println

* remove outdated cleanup config

* update benchmarks

* Add WeightInfo

* enable runtime-benchmarks feature flag

* complete swap benchmark

* add parachains and onboarding into westend

* add benchmarks and genesis

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=auctions --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=slots --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* fix benchmark execution

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=crowdloan --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=paras_registrar --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* Use `new_raise_len` in crowdloan on_initialize

* Update paras_registrar.rs

* fix westend merge

* impl on_swap for crowdloan

* Check fund exists before create

* update for crowdloan sig

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=crowdloan --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* slots on_initialize

* use integration tests environment for benchmarks

* fix hrmp event

* auction on_initialize

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=auctions --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* fix storage name in auctions

* add auction_index to winning data

* winning data takes into account current auction index

* remove println

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=auctions --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=slots --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* Revert "add auction_index to winning data"

* PastRandomness.

* Fixes

* Use new randomness

* fix use of randomness in auctions and runtime config

* expose consts

* fix auction test

* add deposit per byte for para registration

* basic swap integration test

* make swap test more comprehensive

* Add WinningVec for easier retrieval in the front-end.

* clean up `WinningVec` at the end

* Add event for when a new best bid comes in

* Fix propagation of winners in ending period

* fix benchmarks, refund weight in dissolve

* fix unused

* remove some TODOs

* setup opaque keys for paras in westend

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=crowdloan --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* remove unused

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=auctions --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* back to regular runtime config

* use saturating math where user input can be

* better first slot check

* Update runtime/common/src/claims.rs

* update westend onswap impl

Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Parity Benchmarking Bot <[email protected]>

* update ring to 0.16.20 (#2626)

* Remove TODO from substrate#2986 (#2628)

* Companion for sub/8176 (#2622)

* Merge

* Fixes

* Fix build

* remove dep.

* undo dep.

* upadte substrate

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=polkadot-dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/polkadot/src/weights/

* Fix lock

* revert lock; cargo update -p sp-io

* from_rational_approx -> from_rational

* Silence more warnings

Co-authored-by: Gav Wood <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Parity Benchmarking Bot <[email protected]>

* Add /data symlink to Docker containers (#2627)

* add /data symlink to Docker

* fix comments

* Fix wrong deposit amount in council voters. (#2562)

* Fix wrong deposit amount in council voters.

* Fix some build

* make it all compile.. so far.

* Fix

* break build

* Okay fix it again

* re benchmark  (#2630)

* Change something

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=polkadot-dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/polkadot/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=polkadot-dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/polkadot/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=kusama-dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/kusama/src/weights/

* cargo run --release --features=runtime-benchmarks -- benchmark --chain=kusama-dev --steps=50 --repeat=20 --pallet=pallet_election_provider_multi_phase --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/kusama/src/weights/

Co-authored-by: Parity Benchmarking Bot <[email protected]>

* Reduce number of active leaves at startup (#2631)

Currently we will take all leaves and give that to the overseer on
startup, but this is a bad idea when the finality is lagging for
example. There can be many of unfinalized leaves, we don't even need to
look at anymore. To solve this, the pr adds a maximum of 4 leaves we
forward to the overseer and the pr also checks that we only pass uncles
of the best block.

* yet another set of logging improvements (#2638)

* Polkadot companion for Substrate PR #7640 (Store multiple Justifications per block) (#2358)

* service: update for substrate PR #7640

* update substrate

* Add Pallet Babe to Integration Tests Runtime

Co-authored-by: André Silva <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>

* Companion for Substrate#8386 (#2634)

* Companion for Substrate#8386

paritytech/substrate#8386

* "Update Substrate"

Co-authored-by: parity-processbot <>

* Request based collation fetching (#2621)

* Introduce collation fetching protocol

also move to mod.rs

* Allow `PeerId`s in requests to network bridge.

* Fix availability distribution tests.

* Move CompressedPoV to primitives.

* Request based collator protocol: validator side

- Missing: tests
- Collator side
- don't connect, if not connected

* Fixes.

* Basic request based collator side.

* Minor fix on collator side.

* Don't connect in requests in collation protocol.

Also some cleanup.

* Fix PoV distribution

* Bump substrate

* Add back metrics + whitespace fixes.

* Add back missing spans.

* More cleanup.

* Guide update.

* Fix tests

* Handle results in tests.

* Fix weird compilation issue.

* Add missing )

* Get rid of dead code.

* Get rid of redundant import.

* Fix runtime build.

* Cleanup.

* Fix wasm build.

* Format fixes.

Thanks @andronik !

* Companion for #8372 (Replace 'Module' with 'Pallet' in construct_runtime macro) (#2629)

* Replace 'Module' with 'Pallet'.

* "Update Substrate"

* fix babe usage

* fix benchmark

Co-authored-by: parity-processbot <>
Co-authored-by: thiolliere <[email protected]>

* integrate faster erasure code (#2608)

Breaks compatibility for distributing PoV and PersistentValidationData between validators.

Ref #2442

* Update for the new substrate client API (#2570)

* Update for the new substrate client API

* Code review suggestions

* Update substrate

* Improve the logging (#2645)

* Don't accept incoming connections for collators (#2644)

* Don't accept incoming connections for collators

on the `Collation` peer set.

* Better docs.

* fix reconstruct fuzzer name

* make script more robust

* REVERTME: test run

* REVERTME: test run II

* Revert "REVERTME: test run II"

This reverts commit 58375df.

* Revert "REVERTME: test run"

This reverts commit 9759cb6.

Co-authored-by: Robert Habermeier <[email protected]>
Co-authored-by: Gavin Wood <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Parity Benchmarking Bot <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Martin Pugh <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Jon Häggblad <[email protected]>
Co-authored-by: André Silva <[email protected]>
Co-authored-by: Robert Klotzner <[email protected]>
Co-authored-by: Shaun Wang <[email protected]>
Co-authored-by: Bernhard Schuster <[email protected]>
Co-authored-by: Arkadiy Paronyan <[email protected]>
@kianenigma kianenigma added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Apr 19, 2021
@kianenigma
Copy link
Contributor Author

kianenigma commented Oct 31, 2021

More description of what went wrong:

In runtime 28 (first release containing the faulty #1719), we wrote a wrong deposit amount in electionsPhragmen.voting field of all voters. The real deposit (which was already reserved) was 5 DOTs, but we recorded 50 milli-DOTs. This means that the chain had reserved 5 DOTs, but the electionsPhragmen pallet thought that it had reserved 50 milli-DOTs from all voters.

This is problematic for two reasons:

  1. when you submit vote, your new deposit is calculated based on the number of members that you vote for. Then the difference of this value compared to your current recorded deposit (in electionsPhragmen.voting) is either reserved or unreserved.
  2. when you submit remove_voter, the amount that you have in electionsPhragmen.voting is unreserved.

In runtime 30 we partially fixed this. We iterated over all voters, and if any of them had 50 milli-DOTs recorded as deposit, we changed it to 5 DOTs.

This means that:

  • if you never changed your voting between runtime 27 and 30, then you would be fine, and you should have correct deposit values now.

  • If you submitted a vote in this period (and henceforth) for the first time, then the correct deposit was set for you.

  • If you submitted vote in this period, then the chain computed a value for your deposit based on fn deposit, which is certainly more than 5 DOTs, and since that value is certainly more than your recorded 50 milli-DOTs, a large amount reserved again. Afterwards, a correct deposit value is written for you. Any further interaction is then correct. The damage here is: 4.95 DOTs are potentially left in the reserved that need to be moved to free balance now. Example: Alice has 5 DOTs reserved, and the pallet thinks she has 0.05 DOTs reserved. She votes again, and the new deposit is 5.5 DOTs. The pallet should have reserved 0.5 DOTs, but in reality it did 5.45 DOTs, 4.95 of which is incorrect.

  • If you submitted remove_voter, you are basically facing the last part of the previous scenario. Alice has reserved 5 DOTs, but 0.05 is recorded in electionsPhragmen.voting. When she leaves, 0.05 DOTs are unreserved, and 4.95 are left.

An easy way to approach all the above would be to check this: If you submitted either of vote or remove_voter in the [28->30] runtime version period, and your recorded deposit on-chain was 50 milli DOTs, then certainly you ended up in a bad state and some deposit amount (4.95) was lost.


I will conduct an experiment to make a list of accounts affected, with the amount of funds that they have mistakenly locked in their reserved balance. Any other community member who likes to replicate this and cross-check their results with mine, I'd be happy to vouch for an on-chain tip in return ;)

@emielsebastiaan
Copy link

emielsebastiaan commented Nov 3, 2021

We found 189 extrinsics of type ElectionsPhragmen.vote for polkadot runtimes 28, 29 & 30.
We found 47 extrinsics of type ElectionsPhragmen.remove_voter for polkadot runtimes 28, 29 & 30.

Data set here: https://gist.github.com/emielvanderhoek/0a6cf51393e8d22de364b777f98cd453

Please note we have not looked at nested calls, like: batches, proxies, multisig & schedules. These could theoretically have triggered council election votes too.

Lmk if this helps.

@emielsebastiaan
Copy link

Updated gist: https://gist.github.com/emielvanderhoek/0a6cf51393e8d22de364b777f98cd453

We found 49 successfully executed extrinsics of type ElectionsPhragmen.vote for polkadot runtimes 28, 29 & 30.
We found 35 successfully executed extrinsics of type ElectionsPhragmen.remove_voter for polkadot runtimes 28, 29 & 30.

@ical10
Copy link

ical10 commented Nov 3, 2021

Hi @kianenigma, I'd be happy to contribute, but as far I can digest, we will need to check those two types of actions related to voting. Am I correct?
I can start looking at the extrinsics that @emielvanderhoek provided, but then I don't know what to look for and how to do it (perhaps using calls on the js api?).
Please advise on how to proceed.

@kianenigma
Copy link
Contributor Author

My description above is pretty clear about what we are looking for, can you tell me exactly which part of it you don't understand, or need help with?

Note that I have done an analysis myself and don't want to push you too much in the direction of replicating what I did. Instead, Ideally, I want someone else to come up with a different way of doing the analysis independently.

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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants