mystenlabs / mysten-infra Goto Github PK

Investigate building support for the Internet Gateway Device Protocol (IGD). This enables automatic setup of port-forwarding for peers behind some NAT-enabled routers.

Currently the Map trait has not default implementations for the multi_ methods, more specifically for the:

• multi_get
• multi_insert
• multi_remove

it would be beneficial to offer some default implementations based on iteration for the trait. At the moment the DBMap which implements the Map trait offers its own atomic implementations for the above methods. Having the iteration based variants will allow other structs to basically fallback on the default (non atomic) versions.

Throwing a system-wide panic is not an option: the system relies on no more than 33% of nodes failing.
To enable graceful failures, and restart-ability, we are investigating recoverable panic framework via component restarts which limit the scope of a panic to its enclosing task and provides sufficient context to the caller. This helps contain panic blast radius, mitigates node crashes, and still gives developers a familiar paradigm for example we replace assert with mysten_assert which manages error propagation before a panic.

For example scope_guard allows recovery if inner panics

Need convenience functions to clear all in DB

At the moment, our ServerCertVerifier / ClientCertVerifier accept a self-signed certificate that's signed precisely by our expected key.

Workers are authenticated by their primary and do not pre-share their keys, which they generate in an ad-hoc manner.

We would like to have a ServerCertVerifier / ClientCertVerifier that accepts a certificate signed by a specific expected key (the key of the primary).

#103 introduced macros which equip some tables with the ability to dump tables (#116 added some functionality to this).

One important TODO remains on this particular topic: the dump/count_keys capability is made for DBs opened as secondary.

It's not necessarily clear to me that we need the feature of opening a DB as R/W through macros, and if it is it's not clear that we need one implementation with both :

• open as primary,
• open as secondary.
  Rather, it might make more sense to have two structs, one that has read_only + dump capability, and one that has R/W opening and no dump.

What we are doing with our ClientCertVerifier / ServerCertVerifier is accepting one specific key. We can be contacted on the same interface by any of the valid primaries we expect.

We want to extend our "one pre-shared-key" approach only to "an array of pre-shared keys" (the Committee of authorities rather than one specific expected one).

This should:

1. take #97 into account and extend the logic implemented there,
2. take priority over #97 if we have to solve one of these issues rather than both.

See MystenLabs/narwhal#671
We need better control of DBs for optimization & resource mgmt

Currently in order to perform a cross column family batch request, one has to use the DBMap struct to achieve this. An example is shown here:

however, for codebases that we use the Store struct exclusively for our interaction with the storage, there isn't a way to achieve this.

As part of this issue a way should be implemented to allow us perform cross-column family batch requests.

All other skip_to iterator functions return a Result, except for skip_to_last().

Should this also return a result to detect DB faults?

We sometimes retrieve items from the databases just to send them over a network interface right after, and would like to avoid the deserialization-serialization:
https://github.com/MystenLabs/narwhal/blob/410ed5d51b7bd2a1e67fd8b2960986c39be58da9/primary/src/helper.rs#L61

We should offer an interface that allows shipping the raw bytes to their destination directly.

https://github.com/MystenLabs/mysten-infra/blob/main/typed-store/src/rocks/mod.rs#L289
It can never fail.

The typed_store is a thin typed layer over an (untyped, bit-string-manipulating) DB, aimed at making it look like a HashMap.
The pattern of usage for it is to attach several "tables" for the DB by reopening the DB at various ColumnFamily names, leading to the following pattern (which blows up in production):

This is verbose, but cannot be shortened by a function taking in an iterator (the polymorphic nature of reopen would require that iterator to be an HList. However, this could be a relatively straightforward declarative macro.

Multi-insert and multi-remove are current simulated by using batches however convenience functions could wrap these features without exposing the caller to batches.

although I wonder why we don't just have Iter return a stream, so that the caller can turn it into an iterator and use the full panoply of iterator methods on the result.

"Use the panoply of iterator methods" is confusing me: are you calling for a return of an fully-in-memory object that happens to have iterator semantics (in which case I would note HashMap implements IntoIterator)? Or are you instead calling for an Iterator object that would happen to stream values in memory, one by one?

If the later, the answer of why this is difficult (aka "I don't have time") is because:

1. Iterator is a trait object, so it's probably much easier to return this as a method on rocksdb::DBMap than trait::Map,
2. we want this to be accessible from an async method (.iter), which by pattern requires the return going through a channel, which requires pretty annoying constraints on the pointer to the iterator (essentially pinned stack allocation),
3. and then the Iterator itself does not yield asynchronously.

Past those trifling details I suspect your mind is in the right place, though and that we want to turn this API into one that returns a futures::Stream. Want to open an issue for that?

/cc @laura-makdah for why this API may end up deprecated soon-ish, and what it may be replaced by.

Originally posted by @huitseeker in #92 (comment)

Actually I'm not sure this is going to work.
I can connect with Tokio-console to a Sui-node binary compiled by these changes, and with TOKIO_CONSOLE=true env var. However, no tasks are displayed.

I followed all the recommendations in https://docs.rs/console-subscriber/0.1.7/console_subscriber/#enabling-tokio-instrumentation

You have to enable TRACE logs for Tokio and tracing, which just spews an unacceptable amount of logs. And even after that I still am not getting any instrumentation. Granted I had no transactions, but I should be able to see something happening just with consensus and checkpoints, etc.

Originally posted by @velvia in MystenLabs/sui#4438 (comment)

• multi_get was added in #2
• its argument is a slice of values, leading to value copies, because we serialize each of these before passing them to the DB:
  multi_get(&self, keys: &[K]) -> Result<Vec<Option<V>>>
• We should instead take an iterator with references or values for keys at Item (std::borrow::Borrow is useful here),
• inspiration can be gleaned by the type of the delete_batch function, which takes the same sort of argument:
  

  mysten-infra/typed-store/src/rocks/mod.rs

  Lines 164 to 168 in 5221b8f

  	pub fn delete_batch<J: Borrow<K>, K: Serialize, V>(
  	mut self,
  	db: &DBMap<K, V>,
  	purged_vals: impl IntoIterator<Item = J>,
  	) -> Result<Self, TypedStoreError> {

• this change should then be propagated downstream to packages using this.

#22 introduced the Certifiable trait, and introduced the Ed25519 implementation of it.

This allows using the Ed25519 signature scheme (as implemented by ed25519_dalek) conveniently with TLS certificates expecting pre-shared public keys.

The self-signed discipline, and the rustls verifiers that embody them, are valid for any valid X509 SubjectPublickKeyInfo format, which can cover several algorithms, notably Ed25519 and ECDSA P-256

It would be nice to have the same convenience implementation of Certifiable for ECDSA P-256. A good crate to start this with is https://github.com/RustCrypto/signatures/tree/master/ecdsa

Opening and using DBMaps require lots of boilerplate code. Example
Other features like dump also need to be hand written and maintained.