Initial implementation of both sns-executor and sns-worker #277

goshawk-3 · 2025-01-30T11:25:07Z

fixes #283

* fix: make retry-acquire conn cancellable

- add decrypt_128 feature - support polling_interval, max_connections CLI args - rollback db txn, if no tasks available - use pg_notify to avoid SQL inject

rudy-6-4 · 2025-02-03T13:37:27Z

fhevm-engine/fhevm-db/migrations/20250131125649_add_large_ct_column.sql

@@ -0,0 +1,4 @@
+ALTER TABLE ciphertexts 


are migrations always the same (ie same db )?
if so we could have a symbolic link instead for the full directory

Or we could just put the files in the fhevm-engine-common crate or somewhere else, but reusable.

I'm not sure why the migrations folder is duplicated in both fhevm-db and coprocessor, but for now, I'm leaving it as is to prevent conflicts with the tfhe-worker changes.

rudy-6-4 · 2025-02-03T13:45:48Z

fhevm-engine/sns-executor/src/executor.rs

+    info!(target: "sns", { count = records.len()}, "Fetched SnS tasks");
+
+    if records.is_empty() {
+        return Ok(None);


why not an empty vect to simplify the type and the code

Just personal preference.

rudy-6-4 · 2025-02-03T13:48:22Z

fhevm-engine/sns-executor/src/executor.rs

+
+/// Processes the tasks by decompressing and transforming ciphertexts.
+fn process_tasks(tasks: &mut [SnSTask], keys: &KeySet) -> Result<(), Box<dyn std::error::Error>> {
+    set_server_key(keys.public_keys.server_key.clone());


It can't be multi-thread with that. (but multi-process is ok). Is it temprorary ? The interface where you give the key at the operation is not avail ? (there is one but could be private)

which operation require the set_server_key ?
maybe it could be placed just before it or a comment could say so.

set_server_key is required by decompress_ct which is called in a loop. That's why set_server_key is called once before the loop starts.

Depending on the strategy for this issue #284, we may opt for setting it in https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.on_thread_start

rudy-6-4 · 2025-02-03T13:56:03Z

fhevm-engine/sns-executor/src/executor.rs

+    db_txn: &mut Transaction<'_, Postgres>,
+    db_channel: &str,
+) -> Result<(), Box<dyn Error>> {
+    sqlx::query("SELECT pg_notify($1, '')")


I have seen syntax query!("NOTIFY blabla") . Could be more clear but doesn't preserve case.

It's a config variable/notify_channel that is passed here. The pg_notify is here to avoiding SQL inject

rudy-6-4 · 2025-02-03T14:10:47Z

fhevm-engine/sns-executor/src/switch_and_squash.rs

+    pub ksk: LweKeyswitchKey<Vec<u64>>,
+}
+
+pub trait AugmentedCiphertextParameters {


It's just a way to extend Ciphertext from tfhers locally ?
I don't see the trait use elsewhere. Should it be public ?
If not maybe accessing the self.message_modulus.0 directly avoids this trait.

The impl of the trait provides total_block_bits function used in to_large_ciphertext_block.
It's needed for decrypt_128 feature as well.

Also, I'd like to avoid any refactoring in SnS algorithm until we implement this #286.

dartdart26

Looks good, just added a few comments/questions!

dartdart26 · 2025-02-03T14:01:20Z

fhevm-engine/fhevm-db/migrations/20250131125649_add_large_ct_column.sql

@@ -0,0 +1,4 @@
+ALTER TABLE ciphertexts 


Or we could just put the files in the fhevm-engine-common crate or somewhere else, but reusable.

dartdart26 · 2025-02-03T14:05:35Z

fhevm-engine/sns-executor/Cargo.toml

+hex = "0.4"
+
+aligned-vec = "0.5.0"
+num-traits = "=0.2.19"
+sqlx = { version = "0.7", features = ["runtime-tokio", "tls-rustls", "postgres", "uuid"] }
+
+serde_json = "=1.0"


Nit: Could combine these under "crates.io" dependencies.

Also, we could use sqlx from the workspace dependency.

Indeed, I'll move it in the workspace with the next PR.

dartdart26 · 2025-02-03T14:08:59Z

fhevm-engine/fhevm-engine-common/src/types.rs

@@ -389,6 +391,28 @@ impl SupportedFheCiphertexts {
        }
    }

+    pub fn to_regular_ciphertext(self) -> BaseRadixCiphertext<Ciphertext> {


Nit: Wondering if the name "regular" is the best. But I am not sure what ".0" is in this case, is it "regular"?

.0 is actually the Ciphertext64. In kms-core SnS examples, it's also called small_ct. Maybe we can rename this into fn to_ciphertext_64()

dartdart26 · 2025-02-03T14:14:53Z

fhevm-engine/sns-executor/src/bin/sns_worker.rs

+    if let Err(err) = sns_executor::run(keys, &conf, cancel_rx).await {
+        tracing::error!("Worker failed: {:?}", err);
+    }


Which thread(s) execute the SnS?

I mean I guess we expect to run multiple workers, not multiple threads, right?

Rayon thread-pool threads. In to_large_ciphertext, there is par_iter

let res = blocks .par_iter() .map(|current_block| self.to_large_ciphertext_block(current_block)) .collect::<anyhow::Result<Vec<Ciphertext128Block>>>()?;

Ah, yes, it is multithreaded itself!

It's multi-threaded over blocks of a single Ciphertext64 . There is also issue to reconsider this.

#287

rudy-6-4

Could have a test just showing that the part extracted (the algorithm) from tfhers still works. (could be a test copied from tfhers).
E.g. like a simple encrypt / compress / ... / decrypt.

dartdart26 · 2025-02-03T14:27:01Z

fhevm-engine/sns-executor/src/bin/sns_worker.rs

+        },
+    };
+
+    let (_cancel_tx, cancel_rx) = tokio::sync::broadcast::channel(1);


Do we need to signal the _cancel_tx on any signals, etc?

It was overlooked but it's added now

dartdart26 · 2025-02-03T14:55:37Z

fhevm-engine/sns-executor/src/bin/utils/daemon_cli.rs

+    #[arg(long, default_value_t = 10)]
+    pub pg_pool_connections: u32,


Btw, why is the default value 10? Do we need a pool of so many?

TBH, not sure what should be the value for now. I experienced connection issues with pg_pool_connections=1

So, it just follows the recommendation from Coprocessor
See also:

fhevm-backend/fhevm-engine/coprocessor/src/daemon_cli.rs

Line 55 in 685e253

#[arg(long, default_value_t = 10)]

goshawk-3 · 2025-02-03T15:33:52Z

Could have a test just showing that the part extracted (the algorithm) from tfhers still works. (could be a test copied from tfhers).
E.g. like a simple encrypt / compress / ... / decrypt.

This has been tested manually for all TFHE types so far (with Geth and Coprocessor running).

Keys generating, unit-tests and benches must be copied/considered when we implement this: #286

goshawk-3 added 10 commits January 30, 2025 12:54

feat: duplicate switch_and_squash algorithm

b593196

feat: implement a scalable sns executor

6a69313

feat: implement a sns-executor

462be8c

feat: support to_regular_ciphertext for all types

3704e7b

chore: add sns-executor to the workspace

0784bc9

fix: enable to_regular_ciphertext for FheBool

48074ec

fix: update SQL queries to use is_allowed, is_sent columns

49643ba

* fix: make retry-acquire conn cancellable

fix: add db migration for large_ct, is_sent, is_allowed columns

2de5225

docs: add readme.md

0088b60

fix: minor improvements and fixes

9556481

- add decrypt_128 feature - support polling_interval, max_connections CLI args - rollback db txn, if no tasks available - use pg_notify to avoid SQL inject

goshawk-3 marked this pull request as ready for review February 3, 2025 11:56

goshawk-3 requested review from dartdart26, antoniupop, immortal-tofu and rudy-6-4 February 3, 2025 12:45

rudy-6-4 reviewed Feb 3, 2025

View reviewed changes

dartdart26 approved these changes Feb 3, 2025

View reviewed changes

rudy-6-4 approved these changes Feb 3, 2025

View reviewed changes

dartdart26 reviewed Feb 3, 2025

View reviewed changes

goshawk-3 and others added 3 commits February 3, 2025 17:39

Merge branch 'main' into georgi/sns-worker-initial

551dd29

chore: rename to_regular_ciphertext to to_ciphertext64

5a7a07a

fix: handle sigint signal

cfca6c2

goshawk-3 merged commit 95b43ee into main Feb 4, 2025
3 checks passed

goshawk-3 deleted the georgi/sns-worker-initial branch February 4, 2025 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of both sns-executor and sns-worker #277

Initial implementation of both sns-executor and sns-worker #277

goshawk-3 commented Jan 30, 2025 •

edited

Loading

rudy-6-4 Feb 3, 2025

dartdart26 Feb 3, 2025

goshawk-3 Feb 3, 2025

rudy-6-4 Feb 3, 2025 •

edited

Loading

goshawk-3 Feb 3, 2025

rudy-6-4 Feb 3, 2025

rudy-6-4 Feb 3, 2025

goshawk-3 Feb 3, 2025

rudy-6-4 Feb 3, 2025

goshawk-3 Feb 3, 2025

rudy-6-4 Feb 3, 2025

goshawk-3 Feb 3, 2025

dartdart26 left a comment •

edited

Loading

dartdart26 Feb 3, 2025

dartdart26 Feb 3, 2025

goshawk-3 Feb 3, 2025

dartdart26 Feb 3, 2025

goshawk-3 Feb 3, 2025

dartdart26 Feb 3, 2025

dartdart26 Feb 3, 2025

goshawk-3 Feb 3, 2025

dartdart26 Feb 3, 2025

goshawk-3 Feb 3, 2025

rudy-6-4 left a comment

dartdart26 Feb 3, 2025

goshawk-3 Feb 4, 2025

dartdart26 Feb 3, 2025

goshawk-3 Feb 3, 2025

goshawk-3 commented Feb 3, 2025

		#[arg(long, default_value_t = 10)]
		pub pg_pool_connections: u32,

Initial implementation of both sns-executor and sns-worker #277

Initial implementation of both sns-executor and sns-worker #277

Conversation

goshawk-3 commented Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudy-6-4 Feb 3, 2025 • 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

Choose a reason for hiding this comment

dartdart26 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudy-6-4 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

goshawk-3 commented Feb 3, 2025

goshawk-3 commented Jan 30, 2025 •

edited

Loading

rudy-6-4 Feb 3, 2025 •

edited

Loading

dartdart26 left a comment •

edited

Loading