Skeleton and basic tests for confidential store engine #25

Ruteri · 2023-09-06T15:44:00Z

📝 Summary

Readme diff: https://github.com/flashbots/suave-geth/pull/25/files?short_path=b335630#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

Adds a new structure - ConfidentialStoreEngine which manages confidential store interfaces (storage and synchronization).
Adds a simple redis (and miniredis) implementation of confidential data store and synchronization protocol. Each Store is now propagated through the transport to other nodes.
Adds a message signer based on AccountManager - AccountManagerDASigner. This signer signs messages exchanged through confidential store transport layer.

Changes to the Bid:

Changes Id of the Bid structure to be uuidv5 (SHA-1) of the bid
Adds Salt to the Bid structure, a random uuid
Adds AllowedStores to the Bid structure, an allowlist for which execution nodes are able to modify the bid's data in confidential store

Other notable changes:

Refactors how suave stuff is handled in the interpreter - introduces SuaveContext in place of the previous SuaveExecutionBackend. Removes SetConfidentialInputs function.
Simplifies ConfidentialStoreBackends, moves all of the validation logic to the new ConfidentialStoreEngine

📚 References

I have seen and agree to CONTRIBUTING.md

… contracts

… engine

ferranbt · 2023-09-18T07:07:13Z

cmd/utils/flags.go

@@ -516,6 +516,18 @@ var (
 		Category: flags.SuaveCategory,
 	}

+	SuaveConfidentialPubsubRedisEndpointFlag = &cli.StringFlag{


What is the difference between these two flags? Could they be merged?

They can't be merged - one is the backed for storing data (redis owned by whoever is running the node), and the other is a transport layer - owned by whoever is trusted to run the pubsub. Those are two different redis instances, the transport one is shared among multiple nodes

ferranbt · 2023-09-18T07:11:28Z

suave/cmd/suavecli/Dockerfile.suavecli

@@ -0,0 +1,20 @@
+# Build in a stock Go builder container


Are we running suavecli as a docker container now?

I'll remove the whole docker setup for now and we should think about how to best re-introduce it later on

ferranbt · 2023-09-18T07:14:13Z

suave/e2e/redis_transport_test.go

+	mrStore2 := miniredis.RunT(t)
+	mrPubSub := mrStore1
+
+	fr1 := newFramework(t, WithExecutionNode(), WithRedisStoreBackend(mrStore1.Addr()), WithRedisStoreTransport(mrPubSub.Addr()))


Would it be possible to do something like this:

Suggested change

fr1 := newFramework(t, WithExecutionNode(), WithRedisStoreBackend(mrStore1.Addr()), WithRedisStoreTransport(mrPubSub.Addr()))

fr1 := newFramework(t, WithExecutionNode(), WithRedisConfidentialStore())

The Framework is intended to provide a high-level abstraction about the deployment of the chain. That includes any setup required.

Hm, yeah that's true

I changed the store backend to work how you proposed, but the same thing won't quite work for the transport, as it requires a shared redis. I made a helper function that spins up a miniredis instance and returns an opt that is then passed to both frameworks - see if you like it better this way.

metachris · 2023-09-18T08:08:43Z

suave/backends/redis_pubsub.go

+	ch := make(chan suave.DAMessage, 16)
+
+	go func() {
+		for r.ctx.Err() == nil && ctx.Err() == nil {


if one of these errors is not nil, would it get logged somewhere?

generally on this function, i think we might need to add reconnect logic in case the redis connection drops 🤔

Adjusted how context is handled

dmarzzz · 2023-09-18T09:24:38Z

suave/sol/standard_peekers/bids.sol

@@ -25,14 +25,14 @@ contract AnyBidContract {

 contract BundleBidContract is AnyBidContract {

-	function newBid(uint64 decryptionCondition, address[] memory bidAllowedPeekers) external payable returns (bytes memory) {
+	function newBid(uint64 decryptionCondition, address[] memory bidAllowedPeekers, address[] memory bidAllowedStores) external payable returns (bytes memory) {


can't we treat bidAllowedStores similar to executionNodes specified when the user originally sends tx and keep out of this level?

Hm, so the big difference is that the bid's Id (hash) has to take into account the allowed stores -- unlike execution node address, more like the allowed peekers.
I think that having in mind the direction in which we move, this will supersede execution node address, and I think it should be at the contract level

dmarzzz · 2023-09-18T09:29:23Z

suave/core/types.go

 }

 type ConfidentialEthBackend interface {
 	BuildEthBlock(ctx context.Context, args *BuildBlockArgs, txs types.Transactions) (*engine.ExecutionPayloadEnvelope, error)
 	BuildEthBlockFromBundles(ctx context.Context, args *BuildBlockArgs, bundles []types.SBundle) (*engine.ExecutionPayloadEnvelope, error)
 }

-type BuildBlockArgs = types.BuildBlockArgs
+type PubSub interface {


I think Topic is a better name

Or similar pattern to this https://github.com/prysmaticlabs/prysm/blob/d8e6d2cb2ebfa7b288477f98a35470c8b99f2f4e/beacon-chain/p2p/service.go#L71 and this https://github.com/libp2p/go-libp2p-pubsub/blob/d13e24ddc9f288557732c9c02b143c09b86c6778/topic.go#L26

Renamed to StoreTransportTopic

core/vm/suave.go

dmarzzz · 2023-09-18T13:16:10Z

suave/backends/redis_pubsub.go

+	}
+)
+
+type RedisPubSub struct {


I think you also need to include a UUID from the node. Last time I worked with redis pubsub, it would pub it's own message back to itself and you are meant to filter these out with the UUID.

I was thinking to maybe set it to the execution node address, but that would mean nodes can't share it. Uuid is easily spoofed, but we could use both as a way to cut out the messages we send

Check if you like it better

dmarzzz · 2023-09-18T13:22:12Z

suave/backends/redis_store_backend.go

+
+func (r *RedisStoreBackend) Store(bid suave.Bid, caller common.Address, key string, value []byte) (suave.Bid, error) {
+	storeKey := formatRedisBidValueKey(bid.Id, key)
+	err := r.client.Set(r.ctx, storeKey, string(value), time.Second).Err()


We're not putting a TTL/expiry on this. I think that might be okay for now, I would rather worry about Redis getting too large than it expiring out from under me while testing. But we do need to come to consensus on some TTL before testnet and advertise publicly.

Yeah, I've made it a day for now

metachris · 2023-09-19T09:50:56Z

suave/backends/miniredis_backends.go

+		return fmt.Sprintf("bid-%x", bidId)
+	}
+
+	formatMiniredisBidValueKey = func(bid suave.Bid, key string) string {


reuse across backends?

Yeah, I don't think there's any benefit to making miniredis different. Unified

metachris · 2023-09-19T09:51:50Z

suave/backends/redis_store_backend.go

+	"github.com/go-redis/redis/v8"
+)
+
+var ffStoreTTL = 24 * time.Hour


24 could be an env var 🤔

I'd avoid giving a false sense of stability by making this an env var - this is subject to change soon

metachris · 2023-09-19T09:54:03Z

suave/backends/redis_store_backend.go

+
+func (r *RedisStoreBackend) Stop() error {
+	if r.cancel == nil || r.client == nil {
+		panic("Stop() called before Start()")


hmmm perhaps better return an error instead of panic? i think panic shouldn't be used in live code if at all possible (and if used the function should be prefixed with Must)

in this case, you could just silently return too

metachris · 2023-09-19T09:54:48Z

suave/backends/redis_store_backend.go

+	return nil
+}
+
+func (r *RedisStoreBackend) Stop() error {


if error result is unused, remove

It's implementing the node.Lifecycle interface
I'll maybe log the error if any appear?

metachris · 2023-09-19T09:55:28Z

suave/backends/redis_transport.go

+	log.Trace("Rebids pubsub: publishing", "message", message)
+	data, err := json.Marshal(message)
+	if err != nil {
+		panic(fmt.Errorf("could not marshal message: %w", err))


no panic please

On the other hand, under what circumstances would this be the case?
Added a log + early return

metachris · 2023-09-19T09:55:32Z

suave/backends/redis_transport.go

+
+func (r *RedisPubSubTransport) Stop() error {
+	if r.cancel == nil || r.client == nil {
+		panic("Stop() called before Start()")


no panic please

For this one I'm kind of on the fence. This should never be the case, and would only appear if someone introduced a bug - in which case a panic is the quickest way to spot the error.
On the other hand it's not critical so it'd be better to not terminate.
I'll return errors

ferranbt · 2023-09-22T09:03:37Z

suave/core/utils.go

+func calculateBidId(bid types.Bid) (types.BidId, error) {
+	copy(bid.Id[:], emptyId[:])
+
+	body, err := json.Marshal(bid)


Not a blocker but I would consider doing keccak(rlp(bid)) instead to have the same standard as the other hashed objects in the client.

I'd defer to a later point, probably once we'll consider having another implementation - right now the id is localized to this function, and I'm not sure if bid will rlp-encode straight away

.dockerignore

dmarzzz · 2023-09-28T11:48:31Z

suave/core/engine.go

+	transportTopic StoreTransportTopic
+	mempool        MempoolBackend
+
+	daSigner    DASigner


for clarity can we add a comment that "DA = data availability"

dmarzzz

LSTM

dmarzzz · 2023-09-28T11:52:39Z

README.md

@@ -298,26 +303,15 @@ type ConfidentialEthBackend interface {

 The Confidential Store is an integral part of the SUAVE chain, designed to facilitate secure and privacy-preserving transactions and smart contract interactions. It functions as a key-value store where users can safely store and retrieve confidential data related to their bids. The Confidential Store restricts access (both reading and writing) only to the allowed peekers of each bid, allowing developers to define the entire data model of their application!

-The current, and certainly not final, implementation of the Confidential Store is managed by the `LocalConfidentialStore` struct. It provides thread-safe access to the bids' confidential data. The `LocalConfidentialStore` struct is composed of a mutex lock and a map of bid data, `ACData`, indexed by a `BidId`.
+The current, and certainly not final, implementation of the Confidential Store is managed by the `ConfidentialStoreEngine`. The engine consists of a storage backend, which holds the raw data, and a transport topic, which relays synchronization messages between nodes.  
+We provide two storage backends to the confidential store engine: the `LocalConfidentialStore`, storing data in memory in a simple dictionary, and `RedisStoreBackend`, storing data in redis. To enable redis as the storage backed, pass redis endpoint via `--suave.confidential.redis-store-endpoint`.  


perhaps just add a note that Redis is only intended for first testnet and as an upgrade path for a future p2p based protocol

Ruteri force-pushed the confidential-store-engine branch from 3e2ecc7 to 7144245 Compare September 12, 2023 08:14

Ruteri added 9 commits September 13, 2023 12:09

Skeleton and basic tests for confidential store engine

3c9622e

Adds bid creation tx, bid signatures, message signatures

c3fad36

Adjusts lifecycle to match geths, adds redis configs

2431d70

Adds keystore-based message signer

762267d

Adds a devnet setup for the redis-based confidential store transport

2c207e5

Adjusts DA signer, adds allowed stores to newBid in bundles and share…

e71cb0e

… contracts

Merge with upstream

2079fa9

Splits the 32byte BidId into salt+hash

9b8326b

Split docker composes

09195e1

Ruteri force-pushed the confidential-store-engine branch from 3d12d74 to 09195e1 Compare September 15, 2023 08:13

Ruteri added 3 commits September 15, 2023 10:34

Merge upstream, minor cleanup

2be0294

Simplifies store backend, moves all store related validation logic to…

d8d0095

… engine

Refactors how suave stuff is handled in EVM interpreter

975de6a

Ruteri marked this pull request as ready for review September 15, 2023 12:04

Update go.mod

05e2d95

ferranbt reviewed Sep 18, 2023

View reviewed changes

metachris reviewed Sep 18, 2023

View reviewed changes

Ruteri added 2 commits September 18, 2023 13:23

Removes docker multinode setup

cc5cee1

Simplify redis setup in e2e test framework

9e5fc0b

dmarzzz reviewed Sep 18, 2023

View reviewed changes

Ruteri added 7 commits September 18, 2023 16:15

Adjuts how context is handled in store transport implementation

4f8da84

Increase store TTL to 24 hours

2b4b42e

Confidential Engine: drop messages originating from itself

003ee94

Renames pubsub to transport topic

d42cde1

Makes linter happy and adjusts devenv scripts

391355d

Increases timeouts in redis tests

e88250d

Updates go.mod

c054246

Ruteri force-pushed the confidential-store-engine branch from fd926a8 to c054246 Compare September 18, 2023 16:52

Upgrades flashbots/go-utils and its dependencies

83337a8

metachris reviewed Sep 19, 2023

View reviewed changes

Ruteri added 5 commits September 19, 2023 12:42

Updates README.md with changes to Confidential Store

9e854f6

Adjust error handling in Stop() functions

349a8ed

Reuse redis key format in miniredis

055223b

Removes panic from miniredis Publish

df75400

Updates README

9d7c65e

ferranbt reviewed Sep 22, 2023

View reviewed changes

Merge upstream

3fe0fac

metachris reviewed Sep 28, 2023

View reviewed changes

.dockerignore Show resolved Hide resolved

dmarzzz reviewed Sep 28, 2023

View reviewed changes

dmarzzz approved these changes Sep 28, 2023

View reviewed changes

dmarzzz reviewed Sep 28, 2023

View reviewed changes

Add note on redis being temporary

2d250a4

ferranbt self-requested a review September 28, 2023 12:10

ferranbt approved these changes Sep 28, 2023

View reviewed changes

Ruteri merged commit 7568b20 into main Sep 28, 2023

Ruteri deleted the confidential-store-engine branch September 28, 2023 12:31

	fr1 := newFramework(t, WithExecutionNode(), WithRedisStoreBackend(mrStore1.Addr()), WithRedisStoreTransport(mrPubSub.Addr()))
	fr1 := newFramework(t, WithExecutionNode(), WithRedisConfidentialStore())

Skeleton and basic tests for confidential store engine #25

Skeleton and basic tests for confidential store engine #25

Conversation

Ruteri commented Sep 6, 2023 • edited Loading

📝 Summary

📚 References

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruteri Sep 18, 2023 • 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

Ruteri Sep 18, 2023 • 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

Choose a reason for hiding this comment

Ruteri Sep 19, 2023 • 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

Ruteri Sep 19, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarzzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruteri commented Sep 6, 2023 •

edited

Loading

Ruteri Sep 18, 2023 •

edited

Loading

Ruteri Sep 18, 2023 •

edited

Loading

Ruteri Sep 19, 2023 •

edited

Loading

Ruteri Sep 19, 2023 •

edited

Loading