Approval voting overlay db by Lldenaurois · Pull Request #3366 · paritytech/polkadot

Lldenaurois · 2021-06-25T00:21:36Z

This PR refactors the old Action logic in Approval Voting; opting for the more efficient OverlayedBackend pattern proposed by @rphmeier in the ChainSelection subsystem.

Note that the tests could be refactored further to reduce duplication of code, however this will be addressed in the PR for #3365. In addition, the tests in tests.rs could be further refactored to remove the use of TestStore and instead make use of an in-memory dyn KeyValueDB. Due to this PR already getting quite large, I opted to address these items in the next refactor for cleaning up test logic.

Otherwise, would it be preferential to address the superfluous use of TestStore in this PR?

This commit introduces a Backend trait and attempts to move away from the Action model via an OverlayBackend as in the ChainSelection subsystem.

…eight

This commit modifies all tests to ensure tests are passing.

This commit addresses some oversights in the prior commit. 1. Inner errors in backend.write were swallowed 2. One-off write functions removed to avoid useless abstraction 3. Touch-ups in general

This commit removes the TestDB from tests.rs and replaces it with an in-memory kvdb.

node/subsystem/src/lib.rs

node/core/approval-voting/src/backend.rs

ordian

Looks good modulo nits. Not sure if this warrants a burnin on Rococo, but it won't hurt.

node/core/approval-voting/src/backend.rs

node/core/approval-voting/src/ops.rs

node/core/approval-voting/src/lib.rs

drahnr

First iteration looks good - a few small nits, logic check will be done in the second pass.

drahnr · 2021-06-30T04:59:33Z

node/core/approval-voting/src/backend.rs

+		}
+	}
+}
+


Suggested change

use super::ops;

drahnr · 2021-06-30T04:59:54Z

node/core/approval-voting/src/backend.rs

+		&self,
+		candidate_hash: &CandidateHash,
+	) -> SubsystemResult<Option<CandidateEntry>> {
+		super::ops::load_candidate_entry(&*self.inner, &self.config, candidate_hash)


Suggested change

super::ops::load_candidate_entry(&*self.inner, &self.config, candidate_hash)

ops::load_candidate_entry(&*self.inner, &self.config, candidate_hash)

drahnr · 2021-06-30T05:09:59Z

node/core/approval-voting/src/backend.rs

+	inner: &'a B,
+
+	// `None` means unchanged
+	stored_block_range: Option<StoredBlockRange>,


Not a fan of this implicit encoding set with Options, could we not store BackendWriteOp directly? It costs a few extra bytes for the hash being present in addition, but from a clarity pov this would be advantageous.

That's actually a great suggestion... I would suggest we merge this PR and then follow-up in an additional PR, but more than happy to explore the right approach in this PR.

Paging @rphmeier

So basically, instead of a HashMap<K, Option<V>>, you are suggesting we use

HashMap<K, BackendWriteOp> ?

Would the idea there be to have a single map K -> BackendWriteOp? Otherwise I think it could get less clear, not more, because the type system would then express the possibility to have a BackendWriteOp::WriteCandidateEntry in the block_entries map, for example.

Reusing Option for this is a little semantically wrong, but maybe a new enum is a better solution:

enum OverlayValue<T> { Present(T), Deleted } // Semantically - `None` means 'defer to underlying' stored_block_range: Option<OverlayValue<StoredBlockRange>>, // Same here, when calling 'get' for a particular `Hash`. block_entries: HashMap<Hash, OverlayValue<BlockEntry>>,

drahnr · 2021-06-30T05:15:27Z

node/core/approval-voting/src/ops.rs

+		Some(range) => if range.0 >= canon_number {
+			return Ok(())
+		} else {
+			range
+		},


Suggested change

Some(range) => if range.0 >= canon_number {

return Ok(())

} else {

range

},

Some(range) if range.0 >= canon_number => return Ok(()),

Some(range) => range,

drahnr · 2021-06-30T05:19:23Z

node/core/approval-voting/src/ops.rs

+			Some(range) => if range.1 <= number {
+				Some(StoredBlockRange(range.0, number + 1))
+			} else {
+				None
+			}


Suggested change

Some(range) => if range.1 <= number {

Some(StoredBlockRange(range.0, number + 1))

} else {

None

}

Some(range) if range.1 <= number => Some(StoredBlockRange(range.0, number + 1)),

Some(_) => None,

drahnr · 2021-06-30T05:38:48Z

node/core/approval-voting/src/ops.rs

+	// All the block heights we visited but didn't necessarily delete everything from.
+	let mut visited_heights = HashMap::new();
+
+	let visit_and_remove_block_entry = |


nit: If this does not capture state, make it a full fn.

drahnr

LGTM, logic appears good from what I can see 👍

node/core/approval-voting/src/ops.rs

rphmeier · 2021-07-06T05:11:43Z

node/core/approval-voting/src/backend.rs

+
+pub(super) struct DbBackend {
+	inner: Arc<dyn KeyValueDB>,
+	pub(super) config: Config,


Would prefer this to be a getter fn config(&self) -> &Config to protect mutability of the config.

rphmeier · 2021-07-06T05:12:35Z

node/core/approval-voting/src/backend.rs

+		}
+
+		self.inner.write(tx)?;
+		Ok(())


Ok(()) here is redundant if the ?; is omitted from the above line, right?

The difference is the current impl does an implicit type conversion, if the returned Error types are the same, it indeed can be omitted.

node/core/approval-voting/src/ops.rs

node/core/approval-voting/src/persisted_entries.rs

node/core/approval-voting/src/approval_db/v1/mod.rs

node/core/approval-voting/src/backend.rs

node/core/approval-voting/src/lib.rs

rphmeier

Generally looks like the right direction. Could use a few organizational tweaks I've outlined in the comments.

…rlay-db

rphmeier · 2021-07-08T02:28:09Z

node/core/approval-voting/src/backend.rs

+// You should have received a copy of the GNU General Public License
+// along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
+
+//!


Needs better module docs 😅

rphmeier · 2021-07-08T02:29:06Z

node/core/approval-voting/src/persisted_entries.rs

-	backing_group: GroupIndex,
-	our_assignment: Option<OurAssignment>,
-	our_approval_sig: Option<ValidatorSignature>,
+	pub tranches: Vec<TrancheEntry>,


I don't like any of this stuff being pub. What happens when you remove it that is causing issues?

This was due to the code move because the struct is created explicitly in tests. I added a constructor helper and pushed. Should be good to ship

rphmeier · 2021-07-08T15:00:52Z

bot merge

ghost · 2021-07-08T15:00:57Z

Trying merge.

Lldenaurois added 3 commits June 24, 2021 12:24

node/approval-voting: Introduce Backend trait and Overlaybackend

55afad7

This commit introduces a Backend trait and attempts to move away from the Action model via an OverlayBackend as in the ChainSelection subsystem.

node/approval-voting: Add WriteOps for StoredBlockRange and BlocksAtH…

a7a612a

…eight

node/approval-voting: Add load_all_blocks to overlay

5bf68b0

Lldenaurois force-pushed the approval-voting-overlay-db branch from 3173449 to 0d28f06 Compare June 25, 2021 00:21

Lldenaurois added B0-silent Changes should not be mentioned in any release notes I5-tests Tests need fixing, improving or augmenting. A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. labels Jun 25, 2021

Lldenaurois requested review from ordian and rphmeier June 25, 2021 00:23

Lldenaurois force-pushed the approval-voting-overlay-db branch from 0d28f06 to 3f48fad Compare June 25, 2021 00:25

Lldenaurois added the I8-refactor Code needs refactoring. label Jun 25, 2021

node/approval-voting: Get all module tests to pass.

844e56b

This commit modifies all tests to ensure tests are passing.

Lldenaurois force-pushed the approval-voting-overlay-db branch from 3f48fad to 844e56b Compare June 25, 2021 00:50

Lldenaurois added 2 commits June 25, 2021 13:47

node/approval-voting: Address oversights in the previous commit

3e9e5f3

This commit addresses some oversights in the prior commit. 1. Inner errors in backend.write were swallowed 2. One-off write functions removed to avoid useless abstraction 3. Touch-ups in general

node/approval-voting: Move from TestDB to dyn KeyValueDB

12899aa

This commit removes the TestDB from tests.rs and replaces it with an in-memory kvdb.

ordian reviewed Jun 29, 2021

View reviewed changes

node/subsystem/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Jun 29, 2021

View reviewed changes

node/core/approval-voting/src/backend.rs Outdated Show resolved Hide resolved

ordian approved these changes Jun 29, 2021

View reviewed changes

node/core/approval-voting/src/backend.rs Outdated Show resolved Hide resolved

ordian reviewed Jun 29, 2021

View reviewed changes

node/core/approval-voting/src/ops.rs Outdated Show resolved Hide resolved

node/core/approval-voting/src/lib.rs Outdated Show resolved Hide resolved

node/approval-voting: Address feedback

7b9ff0d

Lldenaurois force-pushed the approval-voting-overlay-db branch from fb802a0 to 7b9ff0d Compare June 29, 2021 17:07

node/approval-voting: Add license to ops.rs

46df410

drahnr reviewed Jun 30, 2021

View reviewed changes

drahnr approved these changes Jun 30, 2021

View reviewed changes

node/core/approval-voting/src/ops.rs Show resolved Hide resolved

node/core/approval-voting/src/ops.rs Show resolved Hide resolved

node/approval-voting: Address second-pass feedback

103b873

Lldenaurois force-pushed the approval-voting-overlay-db branch from a70316a to 103b873 Compare June 30, 2021 11:36

Add TODO

7ae0dfa

Lldenaurois mentioned this pull request Jul 1, 2021

Approval voting full subsystem tests #3391

Merged

Merge branch 'master' into approval-voting-overlay-db

ef68ae1