-
Notifications
You must be signed in to change notification settings - Fork 256
Ecash migration module #7976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Ecash migration module #7976
Conversation
8a80eef to
2aac10b
Compare
|
Still need to do a final review myself, approach was:
I think it's ready for review, getting AI review in parallel now. |
2aac10b to
7661025
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
…rastructure Implement core server-side functionality for ecash migration: - Byzantine fault-tolerant consensus for migration transfers - Spend book upload mechanism with authentication - Database schema for transfers and spend book entries - API endpoints for transfer management and upload progress tracking - Code organization improvements (api module, db utilities) This provides the foundation for secure, federated ecash migration.
Implement comprehensive client-side functionality for ecash migration: - Register and manage transfers - Upload spend book data using merkle proofs - Fund transfers with ecash notes - Request activation of migration transfers - Redeem migrated funds This implementation uses merkle tree proofs for efficient and verifiable spend book uploads, replacing the initial naive threshold approach.
1631822 to
d04707c
Compare
Replace unwrap() calls with expect() with clear messages explaining why the operations cannot fail, following project code quality standards.
Convert logging statements to structured format (field = value) as per project standards in the upload_spend_book function.
Add runtime validation to ensure spend book nonces are in strictly ascending sorted order. This prevents silent failures where Merkle tree construction depends on consistent ordering. Panics with a clear error message if sortedness is violated.
Replace inconsistent use of 'liability transfer' with 'transfer' throughout documentation and comments for simplicity and consistency. The module name 'ecash-migration' provides sufficient context.
d04707c to
c73473c
Compare
|
@elsirion update diagram |
| pub struct NaiveThresholdSignature { | ||
| /// A concatenated list of signatures. Has to contain at least the threshold | ||
| /// number of signatures. The remaining signatures can be `None`. | ||
| signatures: Vec<Option<Signature>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to enforce this being ordered, so it is deterministic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why Option is needed. Can't we just assume ones that are not in a vec, are None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, this have to be the same len() as pubkeys and correspond one to one. This is not immediately clear. I would go for HashSet<usize, Signature> which is slightly more compact, but it just a matter of preference, I guess.
| /// A naive threshold signature scheme public key containing all public and the | ||
| /// threshold number of signatures required to verify a signature. | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash, Deserialize, Serialize, Encodable, Decodable)] | ||
| pub struct NaiveThresholdKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Public?
| @@ -0,0 +1,148 @@ | |||
| //! Naive threshold signature scheme implementation using concatenated Schnorr | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could be a separate crate under ./crypto/. More crates = better.
| #[derive(Debug, Clone, Encodable, Decodable, Serialize, Deserialize)] | ||
| pub struct TransferMetadata { | ||
| /// Pre-committed spend book hash (set at creation time) | ||
| pub origin_spend_book_hash: SpendBookHash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if I'm just slow and it was obvious for everyone else, but I just realized that since spend book contains only used nonces, the guardians assuming the liabilities need to explicitly track how many nonces were redeemed per spend book, because the origin guardians could have dumped on them a spent book for which they inflated or they are even still inflating after it was transferred.
| /// Modules are non-compatible with older versions | ||
| pub const MODULE_CONSENSUS_VERSION: ModuleConsensusVersion = ModuleConsensusVersion::new(0, 0); | ||
|
|
||
| /// Unique identifier for an ecash migration transfer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unique how exactly? Sequential?
| Ok(()) | ||
| } | ||
|
|
||
| async fn upload_spend_book_batch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in no way is this authenticated and until we get all entries we can't tell if there was no "corruption"? :/
| ) -> Result<InputMeta, EcashMigrationInputError> { | ||
| Err(EcashMigrationInputError::NotSupported) | ||
| let (transfer_id, note, amount) = match input { | ||
| EcashMigrationInput::RedeemOriginEcash { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so there are special inputs that are accounted and checked against a balance of each transfer. Nice.
| .await | ||
| .is_some(); | ||
| if is_spent_on_origin || is_spent_on_local { | ||
| return Err(EcashMigrationInputError::AlreadyRedeemed(note.nonce)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would split this error, because there's an important difference and caller might want to know which one was it.
| &WithdrawnAmountKey(*transfer_id), | ||
| &withdrawn_amount | ||
| .checked_add(*amount) | ||
| .ok_or(EcashMigrationInputError::Overflow)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect? I mean ... this can't really happen if not a fatal bug.
|
|
||
| /// Prefix for querying all deposited amounts | ||
| #[derive(Debug, Clone, Encodable, Decodable, Serialize)] | ||
| pub struct DepositedAmountPrefix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to consolidate deposit and withdrawal into one record? Generally it seems like one transfer_id -> everything_singular_about_that_transfer_id might be just right? Seems like the only concern would be maybe write-write conflicts with any API calls.
| api_endpoint! { | ||
| GET_TRANSFER_ID_ENDPOINT, | ||
| ApiVersion::new(0, 0), | ||
| async |module: &EcashMigration, context, request: OutPoint| -> TransferId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking that for UX purposes would be good to have a way to look up transfer_id(s) via (self-reported during transfer creation, optional, advisory) FederationId of whos liabilities were transfered. This way clients could potentially easily detect that one federation has funds of some other federation, even without external coordination.
| async fn get_transfer_id( | ||
| &self, | ||
| dbtx: &mut DatabaseTransaction<'_>, | ||
| out_point: OutPoint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't know what is this OutPoint and I don't like it anyway. Couldn't we key requests like this on spend_book_hash or something? That should be unique already.
| TransferId(max_id + 1) | ||
| } | ||
|
|
||
| async fn check_auth( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name could be longer and more specific.
Oh, I see, so this is to auth the requests. This should return some RequestAuthToken that handler functions would take, to make forgetting about auth structurally harder via type system.
| .ok_or(ApiError::not_found("Transfer not found".to_owned()))?; | ||
| let creator_keys = transfer_metadata.creator_keys; | ||
|
|
||
| let auth = context.request_auth().ok_or(ApiError::unauthorized())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... so we ... use the request_auth ... Yyyy... Feels like mixing different concerns etc. and asking for trouble down the line. Would rather take the signature as a separate argument. Could be struct SignedRequest<RequestT> { inner: RequestT, signature: NaiveThresholdSignature } for convenience.
| .into_database() | ||
| } | ||
|
|
||
| async fn open_db_ro(options: &Options) -> fedimint_core::db::Database { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird one-of. Shouldn't we either already have this or use it in more places right away?
| /// | ||
| /// # Errors | ||
| /// - If the origin config file cannot be read or parsed. | ||
| pub async fn read_origin_keyset( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be server side command? Because later down the line we might do migrations and change stuff and then matching client version to server version will be a pain, while it's easy to tell guardian to use the same binary they were using for running the federation in the first place, no? IIRC joshisan wanted to eventually move configs into db, or the other way around - point being that potentially these are moving targets.
We could have some fedimintd export-post-mortem --data-dir ... that would create some versioned (so we can change the format) "export file", and possibly an guardian-authed api endpoint that can do the same thing, or something, and in the client we would be reading the standardized format only, not snoop in the server internals.
| })) | ||
| } | ||
|
|
||
| /// Build Merkle root from a spend book file using streaming with O(log n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing Quest (note for myself): what do we need merkle tree for... ?
| } | ||
|
|
||
| /// Upload a single chunk to all peers with retries. | ||
| async fn upload_chunk_to_peers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_to_guardians ?
| KeySetHash(keyset.consensus_hash_sha256()) | ||
| } | ||
|
|
||
| /// Open a spend book file and return a stream of nonces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a very raw ah-hoc format, without versioning or ability to add helpful metadata etc.
Though if we formalize it into something, we probably want to retain ability to stream the output, which is not always easily doable with standard encoding libraries.
We might do some midle-ground, and e.g. make it text file, that looks a bit like http request
version: 0
key1: value1
key2: value
---
<lines with nonces>
or something. Not a big complication really, but would make it somewhat more flexible. The header part could be even a single line of json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's like biggest life lesson I have to always add version to every protocol and data format one creates. :D
| /// | ||
| /// # Errors | ||
| /// - If the operation does not exist. | ||
| pub async fn subscribe_register_transfer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any eventlog events we should emit?
| })) | ||
| } | ||
|
|
||
| /// Get the controller keypair derived from the module secret. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW. We probably should have some formal way to shut down federation for good. Could be a vote on consensus itself with signatures, plus ability to present a detached closure document. Just so the clients can be detect and inform users that the Federation is now official closed, possibly with some pointers on what to do about the assets. Different PR etc. but just thinking aloud.
|
|
||
| /// Upload the spend book entries in chunks to the destination federation. | ||
| /// | ||
| /// Each chunk is accompanied by a Merkle proof that allows the server to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a merkle tree? Our case seems much simpler. Controllers could just prepare a list of threshold sigs for each batch of nonces, and maybe even we could just hardcode a batch size (to aim at like 1MB chunks), so nonces are uplaoded in signed batches. It's not really server problems to verify a signed batch belongs to the root. If it doesn't, it is sender's problem that they wasted the fee for uploading corrupted data.
| @@ -0,0 +1,553 @@ | |||
| //! Merkle tree implementation for streaming data. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a separate crate if really needed. I'm skipping reviewing because I'm not sure if we really need it, and this PR is already massive. :D
| @@ -1,148 +0,0 @@ | |||
| //! Naive threshold signature scheme implementation using concatenated Schnorr | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhh... That's why merkle tree is used. Because we got rid of threshold sigs ... . So this is now our auth...
|
Need to review the merkle tree thing, and continue reviewing from "test: add ecash liability migration test" |

WARNING: heavy changes still ahead, especially commit history is a bit messy. General consensus architecture should be solidifying now though. A rough outline below: