[fastx] Refactor authority to introduce an order lock and cetificates store #12

gdanezis · 2021-12-01T14:50:33Z

This starts to address issue #11. Specifically I replaces the pending_confirmation and confirmed_log fields within accounts, with a order_locks and certificates field within an authority. This aligns the operation of fastx with the spec.

Renamed accounts field to objects
Rename account_state to object_state
Defined order_lock field in authority and helper functions to init, update and archive locks
Defined a certificates and parent_sync fields in authority, and helper functions to register certificates and maintain indexes.
Updated authority handlers to use the new fields instead of the older pending_confirmation and confirmed_log.
Fixed tests and dependencies.

huitseeker · 2021-12-01T22:28:51Z

fastpay_core/src/authority.rs

+    /// States of fastnft objects
+    objects: BTreeMap<ObjectID, ObjectState>,
+    /// Order lock map maps object versions to the first next transaction seen
+    order_lock: BTreeMap<(ObjectID, SequenceNumber), Option<SignedTransferOrder>>,


What use do I have for two pointers the transactions bringing about two different versions of a same object?

accounts, now named objects store. - Renamed accounts field to objects - Rename account_state to object_state - Defined order_lock structure and helper functions - Added order lock logic to authority handlers - Fixed authoirty tests

- Add certificated field and helper functions - Rename confirmation_log to legacy, and added add_cert call.

- Removed legacy pending_order and replaced it with order_locks - Clean up authority helpers and handlers, and document

huitseeker

Good progress, but I had a few questions / suggestions.

fastpay_core/src/authority.rs

huitseeker · 2021-12-03T14:33:54Z

fastpay_core/src/authority.rs

+        object_id: ObjectID,
+        seq: SequenceNumber,


If you're going to do this, you're inviting multiple clones of the ObjectID, SequenceNumber objects to pair them up before passing a reference to the pair (as you do here with contains_key and get.

There's two options :

live with that and make ObjectID, SequenceNumber implement Copy

require the pair in the API of your functions (or, more to the point of parcimonious copy, a reference to it) wherever you need one. That way the caller has a chance of creating the pair once and reusing it many times, rather than having every callee act as a pair-builder.

Good call, I am now using a type ObjectRef and &ObjectRef to refer to objects, and we already have a to_object_reference on object state to get these.

fastpay_core/src/authority.rs

huitseeker · 2021-12-03T14:37:06Z

fastpay_core/src/authority.rs

+    pub fn archive_order_lock(&mut self, object_id: ObjectID, seq: SequenceNumber) {
+        // Note: for the moment just delete the local lock,
+        // here we can log or write to longer term store.
+        self.order_lock.remove(&(object_id, seq));
+    }


It would be great to have a basic unit test for the lock set / get / archive lifecycle, or at least an issue on writing one.

For the moment I am including a check in the existing tests.

// Check locks are set and archived correctly assert!(authority_state.get_order_lock(&(object_id, 0.into())).is_err()); assert!(authority_state.get_order_lock(&(object_id, 1.into())).expect("Exists").is_none());

fastpay_core/src/unit_tests/authority_tests.rs

huitseeker · 2021-12-03T14:55:10Z

fastpay_core/src/authority.rs

+        // The lock is None, so we replace it with the given order.
+        lock.replace(signed_order);


What's the use case for this (objID, seq) lock to be populated with a None? Are you looking for a Map with default, i.e. this (replacing FooMap with your current favorite):

use std::{borrow::Borrow, collections::FooMap, hash::Hash}; pub struct DefaultFooMap<K, V> { map: FooMap<K, V>, } impl<K, V> DefaultFooMap<K, V> where V: Default, { pub fn new() -> Self { DefaultFooMap { map: FooMap::new() } } pub fn get_mut<Q>(&mut self, v: &Q) -> &mut V where K: Borrow<Q>, Q: Hash + Eq, { self.map.entry(v).or_default() } pub fn get<Q>(&self, v: &Q) -> &V where K: Borrow<Q>, Q: Hash + Eq, { self.map.get(v).unwrap_or_default() } }

I'll leave the customizable default (not reliant on the Default trait) as an exercise to the reader, depends on what you actually need.

Ok, I am leaving an extensive comment about the state of order_locks, which ascribes meaning to the key not existing, the key existing and the value being None, and the key existing and the value being some.

/* Order lock states and invariants Each object in `objects` needs to have an entry in `order_locks` that is initially initialized to None. This indicates we have not seen any valid transactions mutating this object. Once we see the first valid transaction the lock is set to Some(SignedOrder). We will never change this lock to a different value of back to None. Eventually, a certificate may be seen with a transaction that consumes an object. In that case we can delete the key-value of the lock and archive them. This reduces the amount of memory consumed by locks. It also means that if an object has a lock entry it is 'active' ie transaction may use it as an input. If not, a transaction can be rejected. */

One nuance is that the map with default above never needs to materialize the (key, None) record in the map, it just assumes there's a valid None return for any key (or equivalently that any material "key" is "in" the map from the caller's PoV). I was wondering if you could get away with this assumption.

I do not know whether we could use this. But the previous code (which I am slowly refactoring) did not: by placing the pending_confirmation within the accounts it ensured that the lock only exists if the object at this version exists. So I am replicating this here.

by placing the pending_confirmation within the accounts it ensured that the lock only exists if the object at this version exists.

Fair enough.

…r docs.

lxfind

We should also rename handle_account_info_request, AccountInfoRequest and AccountInfoResponse.

We may also want to be more restrictive regarding the lock operations to begin with, and relax them latter if we find valid cases. For example, not even allowing setting the order_lock it the same signed order, not allowing initiating an order_lock that already exists and etc.

I am still trying to understand a few high-level questions, but we could discuss this in Slack:

Why do we prefer to index objects by <id, seq> instead of just (this is essentially the same question as why we prefer to keep objects "immutable").
Why do we prefer to keep an order_lock map in AuthorityState instead of keeping the lock within ObjectState.

lxfind · 2021-12-03T16:41:57Z

fastpay_core/src/authority.rs

 }

 pub struct AuthorityState {
+    // Fixed size, static, identity of the authority and shard


Should committee be moved to the dynamic part? Could it ever change?

lxfind · 2021-12-03T16:44:44Z

fastpay_core/src/authority.rs

+    that is initially initialized to None. This indicates we have not
+    seen any valid transactions mutating this object. Once we see the
+    first valid transaction the lock is set to Some(SignedOrder). We
+    will never change this lock to a different value of back to None.


Suggested change

will never change this lock to a different value of back to None.

will never change this lock to a different value or back to None.

lxfind · 2021-12-03T17:45:35Z

fastpay_core/src/authority.rs

+
+    */
+    /// Order lock map maps object versions to the first next transaction seen
+    order_lock: BTreeMap<(ObjectID, SequenceNumber), Option<SignedTransferOrder>>,


Suggested change

order_lock: BTreeMap<(ObjectID, SequenceNumber), Option<SignedTransferOrder>>,

order_lock: BTreeMap<TransactionDigest, Option<SignedTransferOrder>>,

#### Summary of Changes - migrate some llvm ci pre land tests

gdanezis linked an issue Dec 1, 2021 that may be closed by this pull request

[fastx] Refactor authority structure #11

Closed

gdanezis changed the title ~~Refactor authority to introduce an order lock structure outside the accounts, now named objects store.~~ [Refactor fastpay] Refactor authority to introduce an order lock structure outside the accounts, now named objects store. Dec 1, 2021

gdanezis marked this pull request as draft December 1, 2021 14:55

gdanezis requested review from patrickkuo and huitseeker December 1, 2021 14:56

gdanezis changed the title ~~[Refactor fastpay] Refactor authority to introduce an order lock structure outside the accounts, now named objects store.~~ [fastx] Refactor authority to introduce an order lock structure outside the accounts, now named objects store. Dec 1, 2021

gdanezis changed the title ~~[fastx] Refactor authority to introduce an order lock structure outside the accounts, now named objects store.~~ [fastx] Refactor authority to introduce an order lock and cetificates store Dec 2, 2021

huitseeker reviewed Dec 2, 2021

View reviewed changes

gdanezis marked this pull request as ready for review December 2, 2021 14:46

gdanezis added 5 commits December 3, 2021 09:29

Create certificates store in authority.

81b3f39

- Add certificated field and helper functions - Rename confirmation_log to legacy, and added add_cert call.

Clean up older order locks & Removed confirmed_log legacy structure

86a9ed2

Make clippy happier

3b309bd

Refactor pending_order field into order_locks in authority.

99689c2

- Removed legacy pending_order and replaced it with order_locks - Clean up authority helpers and handlers, and document

gdanezis requested a review from lxfind December 3, 2021 09:46

gdanezis force-pushed the athorityrefactor branch from be6014d to 99689c2 Compare December 3, 2021 10:15

huitseeker reviewed Dec 3, 2021

View reviewed changes

Addressed review comments: use objectref, idiomatic lookups and bette…

f5bb29f

…r docs.

gdanezis merged commit 4227861 into main Dec 3, 2021

lxfind reviewed Dec 3, 2021

View reviewed changes

gdanezis deleted the athorityrefactor branch December 13, 2021 19:32

This was referenced Nov 9, 2022

Sui Code Bug or Feature Request #5963

Closed

sui rosetta cant build #5964

Closed

brson pushed a commit to brson/sui that referenced this pull request Apr 30, 2024

ci: add llvm ci pre land (MystenLabs#12)

944d04b

#### Summary of Changes - migrate some llvm ci pre land tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx] Refactor authority to introduce an order lock and cetificates store #12

[fastx] Refactor authority to introduce an order lock and cetificates store #12

gdanezis commented Dec 1, 2021 •

edited

Loading

huitseeker Dec 1, 2021

huitseeker left a comment

huitseeker Dec 3, 2021

gdanezis Dec 3, 2021

huitseeker Dec 3, 2021

gdanezis Dec 3, 2021

huitseeker Dec 3, 2021 •

edited

Loading

gdanezis Dec 3, 2021

gdanezis Dec 3, 2021

huitseeker Dec 3, 2021

gdanezis Dec 3, 2021

huitseeker Dec 3, 2021

lxfind left a comment

lxfind Dec 3, 2021

lxfind Dec 3, 2021

lxfind Dec 3, 2021

		// The lock is None, so we replace it with the given order.
		lock.replace(signed_order);

	will never change this lock to a different value of back to None.
	will never change this lock to a different value or back to None.

	order_lock: BTreeMap<(ObjectID, SequenceNumber), Option<SignedTransferOrder>>,
	order_lock: BTreeMap<TransactionDigest, Option<SignedTransferOrder>>,

[fastx] Refactor authority to introduce an order lock and cetificates store #12

[fastx] Refactor authority to introduce an order lock and cetificates store #12

Conversation

gdanezis commented Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis commented Dec 1, 2021 •

edited

Loading

huitseeker Dec 3, 2021 •

edited

Loading