Approval Voting Database by rphmeier · Pull Request #2162 · paritytech/polkadot

rphmeier · 2020-12-23T20:21:15Z

This change-set became large on its own. This is implementing the described database and operations from the Approval Voting subsystem, against the AuxStore trait.

Main points to review:

The structs match the "Database Schema" section
The start-up logic is fulfilled by fn clear
The finalization logic is fulfilled by fn canonicalize
The ActiveLeavesUpdate logic is fulfilled by fn add_block_entry

Related to #1975

also add some key computation

rphmeier · 2020-12-24T00:17:13Z

The bot added pleasereview before I could :)

burdges · 2020-12-30T09:52:59Z

node/core/approval-voting/src/aux_schema/mod.rs

+	next_wakeup: Tick,
+	our_assignment: Option<OurAssignment>,
+	// `n_validators` bits.
+	assignments: BitVec<BitOrderLsb0, u8>,


We could discuss this feature further, and even if we keep it caching the assignments remains fine, but in my design no-shows giving approval votes can unassign some no show replacements.

That's still maintained in the logic PR that follows, because we always compute a mask over this bit-vec based on the tranches we take.

polkadot/node/core/approval-voting/src/aux_schema/mod.rs

Lines 83 to 93 in b854306

pub(crate) fn assignments_up_to(&self, tranche: DelayTranche) -> BitVec<BitOrderLsb0, u8> {

self.tranches.iter()

.take_while(|e| e.tranche <= tranche)

.fold(bitvec::bitvec![BitOrderLsb0, u8; 0; self.assignments.len()], |mut a, e| {

for &(v, _) in &e.assignments {

a.set(v as _, true);

}

a

})

}

(from the next PR)

burdges · 2020-12-30T09:53:55Z

node/core/approval-voting/src/aux_schema/mod.rs

+	session: SessionIndex,
+	// Assignments are based on blocks, so we need to track assignments separately
+	// based on the block we are looking at.
+	block_assignments: BTreeMap<Hash, ApprovalEntry>,


I suppose the Hash here is the hash of the relay chain block where inclusion happens? It's ambiguous form this comment.

Yes, that's right. In our code-base Hash is block hash and CandidateHash is candidate hash.

Would it be worthwhile to define a BlockHash newtype adjacent to CandidateHash? It won't change anything re: type safety, but it might make it even harder to confuse what each hash represents.

"worthwhile" is always a question of opportunity cost :) . I wouldn't object to such a change, but I don't think it's in the scope of this PR.

Agree, that would be its own thing. I'll extract an issue.

burdges · 2020-12-30T10:02:01Z

node/core/approval-voting/src/aux_schema/mod.rs

+	// A bitfield where the i'th bit corresponds to the i'th candidate in `candidates`.
+	// The i'th bit is `true` iff the candidate has been approved in the context of this
+	// block. The block can be considered approved if the bitfield has all bits set to `true`.
+	approved_bitfield: BitVec<BitOrderLsb0, u8>,


Actually either all bits for which self.candidates.binary_search_by(|probe| probe.0.cmp(&i)).is_ok() holds or else all bits but we set any missing cores initially.

I should double check when these bits get set later.

This bitfield is by candidates, not by cores. So if there were candidates included from cores [1, 3, 7], then the bit-field would have 3 bits corresponding to each of those, respectively. I think all bits is correct with that in mind, but I might misunderstand the purpose of the binary search snippet you posted if it's not for checking existence of the core in the included candidates set.

It does mean that every time we want to mark a candidate as approved, we have an extra search of the candidate set for each block entry a candidate appears in, but that's contiguous memory and would be dwarfed by the cost of writing that metadata to disk.

I only meant that approved_bitfield could be initialized to zero for bits representing existent candidates and one for bits representing non-existent candidates. I just assumed candidates would always be indexed by core in this stage. Appears you're indexing by some other candidate index, which yes makes BitVec's length is equivalent to presetting non-existent candidates to one. I see..

From the outside of this struct, we do lookup by core, but we have to do an internal look-up via the candidates vec first, which is cheap.

burdges · 2021-01-04T21:34:15Z

roadmap/implementers-guide/src/node/approval/approval-voting.md

 #### `OverseerSignal::BlockFinalized`

-On receiving an `OverseerSignal::BlockFinalized(h)`, we fetch the block number `b` of that block from the ChainApi subsystem. We update our `StoredBlockRange` to begin at `b+1`. Additionally, we remove all block entries and candidates referenced by them up to and including `b`. Lastly, we prune out all descendents of `h` transitively: when we remove a `BlockEntry` with number `b` that is not equal to `h`, we recursively delete all the `BlockEntry`s referenced as children. We remove the `block_assignments` entry for the block hash and if `block_assignments` is now empty, remove the `CandidateEntry`.
+On receiving an `OverseerSignal::BlockFinalized(h)`, we fetch the block number `b` of that block from the ChainApi subsystem. We update our `StoredBlockRange` to begin at `b+1`. Additionally, we remove all block entries and candidates referenced by them up to and including `b`. Lastly, we prune out all descendents of `h` transitively: when we remove a `BlockEntry` with number `b` that is not equal to `h`, we recursively delete all the `BlockEntry`s referenced as children. We remove the `block_assignments` entry for the block hash and if `block_assignments` is now empty, remove the `CandidateEntry`. We also update each of the `BlockNumber -> Vec<Hash>` keys in the database to reflect the blocks at that height, clearing if empty.


We keep an checks running however I guess. We should also keep the erasure coded pieces and the outgoing messages sent by blocks for another 24 hours or whatever our XCMP window is.

We keep around the data but don't do any checks after finality anymore.

coriolinus

Some nits, but nothing essential. Looks like a reasonable implementation of a data store backed by AuxStore for the types described in the guide.

coriolinus · 2021-01-05T09:53:13Z

node/core/approval-voting/Cargo.toml

+polkadot-overseer = { path = "../../overseer" }
+polkadot-primitives = { path = "../../../primitives" }
+polkadot-node-primitives = { path = "../../primitives" }
+bitvec = "0.17.4"


nit: bitvec should go among the external dependencies, not among the polkadot dependencies.

coriolinus · 2021-01-05T09:53:39Z

node/core/approval-voting/Cargo.toml

+sp-consensus-slots = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+sp-blockchain = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+
+[dev-dependencies]


nit: text files should have trailing newlines.

node/core/approval-voting/src/aux_schema/mod.rs

node/core/approval-voting/src/aux_schema/tests.rs

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

rphmeier added 27 commits December 10, 2020 10:12

skeleton

21d108e

skeleton aux-schema module

7c0a083

start approval types

f1308f0

start aux schema with aux store

6a6c5c0

doc

7173066

finish basic types

006dfa7

start approval types

557b518

doc

dc55799

finish basic types

fe79b4f

Merge branch 'master' into rh-approval-types

e772603

write out schema types

8c0cef1

add debug and codec impls to approval types

5ec3cfb

add debug and codec impls to approval types

f415724

also add some key computation

add debug and codec impls to approval types

1088ec3

getters for block and candidate entries

3ce7414

grumbles

c9af5a0

remove unused AssignmentId

65f9fed

Merge branch 'rh-approval-types' into rh-approval-voting

321d43b

load_decode utility

910982a

implement DB clearing

76bcfac

function for adding new block entry to aux store

9a04615

start canonicalize implementation

bc6dcc2

more skeleton

834d776

Merge branch 'master' into rh-approval-voting

31e840e

Merge branch 'master' into rh-approval-voting

5d1fafb

finish implementing canonicalize

aebfecc

tag TODO

7faa0c3

rphmeier 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 Dec 23, 2020

github-actions bot 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 Dec 23, 2020

rphmeier added 9 commits December 23, 2020 15:32

implement a test AuxStore

4a49ffb

add allow(unused)

0f1a8d6

basic loading and deleting test

f7d7c49

block_entry test function

0d384f7

add a test for add_block_entry

b07f7a9

ensure range is exclusive at end

ec798fb

test clear()

10613b4

test that add_block sets children

bbe4b0c

add a test for canonicalize

a13d54c

rphmeier requested a review from ordian December 24, 2020 00:38

rphmeier mentioned this pull request Dec 27, 2020

Implement Approval Voting Subsystem #2112

Merged

burdges reviewed Dec 30, 2020

View reviewed changes

burdges reviewed Jan 4, 2021

View reviewed changes

coriolinus approved these changes Jan 5, 2021

View reviewed changes

rphmeier and others added 3 commits January 5, 2021 09:23

Update node/core/approval-voting/src/aux_schema/mod.rs

e336e45

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

Update node/core/approval-voting/src/aux_schema/tests.rs

69fb1b4

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

Update node/core/approval-voting/src/aux_schema/mod.rs

1e4be6e

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

coriolinus mentioned this pull request Aug 24, 2023

Define a BlockHash newtype adjacent to CandidateHash paritytech/polkadot-sdk#981

Open

rphmeier merged commit a4c418a into master Jan 18, 2021

rphmeier deleted the rh-approval-voting-db branch January 18, 2021 16:07

rphmeier mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#4

Merged

	pub(crate) fn assignments_up_to(&self, tranche: DelayTranche) -> BitVec<BitOrderLsb0, u8> {
	self.tranches.iter()
	.take_while(\|e\| e.tranche <= tranche)
	.fold(bitvec::bitvec![BitOrderLsb0, u8; 0; self.assignments.len()], \|mut a, e\| {
	for &(v, _) in &e.assignments {
	a.set(v as _, true);
	}

	a
	})
	}

Conversation

rphmeier commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rphmeier commented Dec 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burdges Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burdges Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rphmeier commented Dec 23, 2020 •

edited

Loading

rphmeier Jan 2, 2021 •

edited

Loading

burdges Dec 30, 2020 •

edited

Loading

rphmeier Jan 2, 2021 •

edited

Loading

burdges Dec 30, 2020 •

edited

Loading