Massive Spend Refactor #1989

grumbach · 2024-07-19T21:19:49Z

BREAKING CHANGE

Continuing off @maqi's redesign of Spends in refactor PR #1930
Redesigns the whole transacting flow from CashNotes all the way to Spend with a simpler design:

Removing:

OfflineTransfer: all you need to make a transaction, signed
TransferInputs : amassed data to make a transaction's inputs
CashNotesAndSecretKey : this for inputs too
TransactionBuilder: this makes a transaction from inputs
CashNoteBuilder: this makes cashnotes from transactions
UnsignedTransfer: this is a special case when we don't have a key to sign the transaction
Transaction, Input, Output: this was in Spends
the middle spend patch
lots of unused error variants
duplicate code
layers and layers of ~~obfuscation~~ abstraction
more than 500 lines of code
...

Replaced by:

UnsignedTransaction: all you need to make a transaction, not signed
SignedTransaction: all you need to make a transaction, signed

What's next

adapt spend_simulation tests that were using the Transactions in Spends @RolandSherwin
implement UnsignedTransaction::verify
unit test UnsignedTransaction::new, UnsignedTransaction::sign
unit test hex serialization for both new types

joshuef · 2024-07-22T01:43:19Z

What's the impact here for OMNI disbursement if any?

sn_client/src/audit/tests/mod.rs

sn_transfers/benches/reissue.rs

sn_transfers/src/transfers/unsigned_transaction.rs

sn_node/tests/spend_simulation.rs

+//             info!("{id} verifying status of spend number({num:?}): {spend:?} : {status:?}");
+//             match status {
+//                 SpendStatus::Utxo => {
+//                     // TODO: with the new spend struct requiring `middle payment`


grumbach · 2024-07-22T14:44:36Z

What's the impact here for OMNI disbursement if any?

This reworks the structure of the Spend and refactors the transaction flow, it shouldn't affect OMNI disbursement

sn_client/src/audit/dag_crawling.rs

sn_client/src/audit/tests/setup.rs

grumbach · 2024-07-23T18:06:26Z

sn_transfers/src/genesis.rs

 pub fn is_genesis_spend(spend: &SignedSpend) -> bool {
    let bytes = spend.spend.to_bytes_for_signing();
    spend.spend.unique_pubkey == *GENESIS_SPEND_UNIQUE_KEY
        && GENESIS_SPEND_UNIQUE_KEY.verify(&spend.derived_key_sig, bytes)
-        && is_genesis_parent_tx(&spend.spend.parent_tx)
-        && spend.spend.amount == NanoTokens::from(GENESIS_CASHNOTE_AMOUNT)
+        && spend.spend.amount() == NanoTokens::from(GENESIS_CASHNOTE_AMOUNT)
 }


If we know the content of the Genesis Spend, maybe also add those values in this test to make sure it's the one and unique genesis? Do we know those values?

&& spend.spend.reason == GENESIS_REASON && spend.spend.ancestors == GENESIS_ANCESTORS && spend.spend.descendants == GENESIS_DESCENDANTS

Assuming GENESIS_SK is destroyed at Network Genesis, this scenario should not happen, but are we ever too safe?

Do we know those values?

Yeah, we do can know these values.

Assuming GENESIS_SK is destroyed at Network Genesis

shall be OK, as GENESIS_SK is actually not used (and shall not be used) after GENESIS_SPEND generated.

sn_client/src/audit/tests/mod.rs

sn_cli/src/bin/subcommands/wallet/hot_wallet.rs

sn_transfers/src/cashnotes/cashnote.rs

maqi · 2024-07-24T13:36:04Z

sn_transfers/src/cashnotes/signed_spend.rs

-    {
+    /// - verifies that the parent_spends contains self as an output
+    /// - verifies the sum of total inputs equals to the sum of outputs
+    pub fn verify_parent_spends(&self, parent_spends: &BTreeSet<SignedSpend>) -> Result<()> {


I am wondering without the middle pay, how the above conditions got all satisfied for the scenario of multiple_inputs to multiple_outputs ?

looks like it now trying to use return multiple cash_note, each cn per recipient, i.e. get inputs distributed to outpus explicitly to resolve this.
So, here, both of the criterias can still be satisfied at the same time.

Yes all this is covered by the distribution algo in UnsignedTransaction

maqi · 2024-07-24T13:39:53Z

sn_transfers/src/cashnotes/signed_spend.rs

+            // check for double spend parents
+            if spends.len() > 1 {
+                error!("While verifying parents of {unique_key}, found a double spend parent: {spends:?}");
+                return Err(TransferError::DoubleSpentParent);


This actually means the spend itself having duplicated ancestors ?
which is different to the scenario that an ancestor got spent already?
maybe it shall give a different name to differiate these two scenarios?

This actually means the spend itself having duplicated ancestors ?

This means one of the spend's parents has multiple entries and is burnt.

maqi · 2024-07-24T13:55:11Z

sn_transfers/src/genesis.rs

 pub fn is_genesis_spend(spend: &SignedSpend) -> bool {
    let bytes = spend.spend.to_bytes_for_signing();
    spend.spend.unique_pubkey == *GENESIS_SPEND_UNIQUE_KEY
        && GENESIS_SPEND_UNIQUE_KEY.verify(&spend.derived_key_sig, bytes)
-        && is_genesis_parent_tx(&spend.spend.parent_tx)
-        && spend.spend.amount == NanoTokens::from(GENESIS_CASHNOTE_AMOUNT)
+        && spend.spend.amount() == NanoTokens::from(GENESIS_CASHNOTE_AMOUNT)
 }


Do we know those values?

Yeah, we do can know these values.

Assuming GENESIS_SK is destroyed at Network Genesis

shall be OK, as GENESIS_SK is actually not used (and shall not be used) after GENESIS_SPEND generated.

sn_transfers/src/transfers/signed_transaction.rs

sn_transfers/src/transfers/unsigned_transaction.rs

maqi · 2024-07-25T14:48:05Z

sn_transfers/src/transfers/unsigned_transaction.rs

@@ -93,11 +111,21 @@ impl UnsignedTransaction {
            })
            .collect();

+        // order inputs by value, re const after sorting
+        let mut cashnotes_big_to_small = available_cash_notes;


we shall try to use the small ones first ?

Ideally we'd like to minimize the amount of Spends created, using big ones first has that effect for now, but it's not optimal over the long run.

In the future I imagine we should create a better algorithm that would optimize that and choose the best options with the closest values as to minimize the amount of spends and avoid using huge Cashnotes for small Txs.

Also, since CashNotes don't have a determined value anymore, we could return the change value to an existing unspent CashNote that would accumulate wealth until it is spent.

All these nice optimizations will not change the API in this PR so that's also nice!

BREAKING CHANGE

BREAKING CHANGE: many types gone, replaced by UnsignedTransaction SignedTransaction

grumbach force-pushed the without_middle_spend branch from 12b7c4d to 9374151 Compare July 22, 2024 14:42

github-advanced-security bot found potential problems Jul 22, 2024

View reviewed changes

grumbach added the Breaking Change label Jul 22, 2024

grumbach commented Jul 22, 2024

View reviewed changes

sn_client/src/audit/dag_crawling.rs Outdated Show resolved Hide resolved

grumbach commented Jul 23, 2024

View reviewed changes

sn_client/src/audit/tests/setup.rs Outdated Show resolved Hide resolved

grumbach commented Jul 23, 2024

View reviewed changes

sn_client/src/audit/tests/setup.rs Outdated Show resolved Hide resolved

grumbach force-pushed the without_middle_spend branch from e4203ee to c104e4b Compare July 23, 2024 17:15

grumbach commented Jul 23, 2024

View reviewed changes

grumbach force-pushed the without_middle_spend branch from 6c2a077 to dd16544 Compare July 23, 2024 18:14

github-advanced-security bot found potential problems Jul 23, 2024

View reviewed changes

sn_client/src/audit/tests/mod.rs Fixed Show fixed Hide fixed

grumbach force-pushed the without_middle_spend branch from 7fc6a99 to 850b28f Compare July 23, 2024 20:03

grumbach marked this pull request as ready for review July 23, 2024 20:44

grumbach enabled auto-merge July 23, 2024 20:55

grumbach requested review from maqi and RolandSherwin July 24, 2024 09:45

grumbach mentioned this pull request Jul 24, 2024

chore!: refactor spend struct #1930

Closed

maqi requested changes Jul 24, 2024

View reviewed changes

maqi mentioned this pull request Jul 25, 2024

Refactor spend struct #1931

Closed

grumbach force-pushed the without_middle_spend branch 2 times, most recently from c8a9195 to 935becf Compare July 25, 2024 14:37

maqi reviewed Jul 25, 2024

View reviewed changes

grumbach force-pushed the without_middle_spend branch from 935becf to e0d8aeb Compare July 29, 2024 13:25

maqi and others added 6 commits July 29, 2024 17:17

chore!: refactor spend struct

abc3df2

BREAKING CHANGE

fix(spend): support multiple inputs with different keys

75afa35

chore(kad): not enough close peers no longer result in error out

b66d5a9

chore(spend_simulation): disable non-cashnote UTXO spend error out

cbde3dc

chore(spend_simulation): disable failing query attempt assertion

9942c03

feat: add support for multi parents in CNR

b187c8f

grumbach added 17 commits July 29, 2024 17:17

refactor: entire transacting flow from CashNote to SignedSpend

2c1ca64

BREAKING CHANGE: many types gone, replaced by UnsignedTransaction SignedTransaction

fix: move tx verification to wallet sign

9a54e12

fix: re enable reissue bench

17d95d2

fix: doctests

178b01c

fix: bench

cabb759

feat: improve network royalties identification for spends

e48af65

fix: spends fixes and security verifications

dd91c1c

chore: improve code quality

386d1c7

fix: dag double spend test

9bd610a

fix: serious error in spend dag double spend branching

d3c9e39

fix: dag crawling invalid source check

fb33e07

fix: re comment uncommented block

fbc6613

chore: update docs for CashNote and spends

dbe8822

feat: fixes and tests for unsigned tx

0f92260

chore: disable flawed tests sending out invalid 0 tx

f574fb5

fix: allow time for double spend to spread in spend test

c2b3bb1

chore: adapt after rebase

9fe6f44

grumbach force-pushed the without_middle_spend branch from ab59db4 to 9fe6f44 Compare July 29, 2024 15:25

test: bump sleep time to 20s

8bcab6d

grumbach force-pushed the without_middle_spend branch from 41193e0 to 8bcab6d Compare July 29, 2024 15:47

grumbach added 2 commits July 29, 2024 19:26

fix: test issue

bab2054

feat: remove output purpose

4643330

joshuef mentioned this pull request Aug 9, 2024

Spend and range atop main #2033

Closed

grumbach mentioned this pull request Aug 27, 2024

feat: Generic payments groundwork #2066

Closed

maqi mentioned this pull request Aug 27, 2024

Docs for currency #2042

Merged

maqi removed the Breaking Change label Aug 28, 2024

maqi approved these changes Aug 28, 2024

View reviewed changes

grumbach added this pull request to the merge queue Aug 28, 2024

Merged via the queue into maidsafe:main with commit e15aae3 Aug 28, 2024
117 of 120 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Massive Spend Refactor #1989

Massive Spend Refactor #1989

grumbach commented Jul 19, 2024 •

edited by maqi

Loading

joshuef commented Jul 22, 2024

grumbach commented Jul 22, 2024

grumbach Jul 23, 2024 •

edited

Loading

maqi Jul 24, 2024

maqi Jul 24, 2024

maqi Jul 25, 2024

grumbach Jul 25, 2024

maqi Jul 24, 2024

grumbach Jul 25, 2024 •

edited

Loading

maqi Jul 24, 2024

maqi Jul 25, 2024

grumbach Jul 25, 2024

Massive Spend Refactor #1989

Massive Spend Refactor #1989

Conversation

grumbach commented Jul 19, 2024 • edited by maqi Loading

Removing:

Replaced by:

What's next

joshuef commented Jul 22, 2024

grumbach commented Jul 22, 2024

grumbach Jul 23, 2024 • 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

grumbach Jul 25, 2024 • 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

grumbach commented Jul 19, 2024 •

edited by maqi

Loading

grumbach Jul 23, 2024 •

edited

Loading

grumbach Jul 25, 2024 •

edited

Loading