feat!: add one-sided coinbase payments #5967

hansieodendaal · 2023-11-18T08:53:18Z

Description

Added normal one-sided and stealth one-sided coinbase transactions. Changes are:

Coinbases can now only be paid to a nominated wallet address directly.
The minotari miner and merge mining proxy will only start if a valid minotari wallet address has been supplied for the particular network.
The miner and merge mining proxy does not depend on the wallet anymore to construct the coinbase; it is done directly.
Corresponding gRPC method (rpc GetCoinbase (GetCoinbaseRequest) returns (GetCoinbaseResponse)) has been removed from the wallet.
A memory-based transactions key manager has been created for use by the miner and merge mining proxy as they do not need persistent storage of keys. This also ensures that new entropy for the generation of keys is created each time the miner or merge mining proxy is started effectively negating any collisions of private keys.
All unused code in the output manager and transaction manager have been removed.

Motivation and Context

See #5930

How Has This Been Tested?

Unit tests
Cucumber tests
System-level tests - Completed

What process can a PR reviewer use to test or verify this change?

Code walk-through
Run cucumber Scenario: Get Transaction Info and review the log files for WALLET_A where the stealth one-sided coinbases will be imported and where payment is made to WALLET_B.

Breaking Changes

None
Requires data directory on base node to be deleted
Requires hard fork
Other - Please specify

Coinbases can now only be paid to a nominated wallet address directly.
Existing wallets need to be recovered into new wallets.

github-actions · 2023-11-20T06:30:07Z

Test Results (CI)

1 249 tests 1 249 ✔️ 12m 46s ⏱️
    39 suites       0 💤
      1 files       0 ❌

Results for commit 2136543.

♻️ This comment has been updated with latest results.

github-actions · 2023-11-20T06:47:03Z

Test Results (Integration tests)

31 tests +31 31 ✔️ +31 15m 1s ⏱️ + 15m 1s
11 suites +11   0 💤 ±  0
  2 files +  2   0 ❌ ±  0

Results for commit 2136543. ± Comparison against base commit 64e375b.

♻️ This comment has been updated with latest results.

Added one-sided and stealth coinbase transactions. Coinbases can now only be paid to a nominated wallet address directly.

stringhandler · 2023-11-21T10:37:19Z

Added exclamation for breaking change

hansieodendaal

System-level testing: Merge mining proxy should panic if the wallet payment address is not set; currently it is logging an error.

hansieodendaal

We need more detail about imported coinbase transactions on the console wallet's transactions pane

hansieodendaal

Many of the imported coinbase transactions in the nominated wallet have the incorrect status. We expect FauxUnconfirmed -> FauxConfrimed, but they end up as FauxUnconfirmed -> Completed. The Completed transactions are then broadcast to the mempool, which is just a further symptom to the actual problem.

SWvheerden

looking good, a few comments

applications/minotari_console_wallet/README.md

SWvheerden · 2023-11-22T07:13:42Z

applications/minotari_console_wallet/src/grpc/mod.rs

-            // The coinbase are technically Inbound.
-            // To determine whether a transaction is coinbase
-            // we will check whether the message contains `Coinbase`.
-            is_coinbase: inbound.message.to_lowercase().contains("coinbase"),


We do not have coinbase transactions anymore; we just import one-sided coinbase UTXOs and make faux transactions

applications/minotari_console_wallet/src/init/mod.rs

base_layer/core/src/transactions/coinbase_builder.rs

applications/minotari_miner/src/run_miner.rs

SWvheerden · 2023-11-22T12:26:08Z

base_layer/wallet/migrations/2023-11-14-131400_coinbase/up.sql

@@ -0,0 +1,69 @@
+-- Any old 'outputs' will not be valid due to the removal of 'coinbase_block_height',


why remove this? we can still add this in?

The coinbase_block_height column was only used to manage coinbase transactions in the transaction service; this is not used anymore due to the removal of conbase transactions.

SWvheerden · 2023-11-22T12:26:18Z

base_layer/wallet/migrations/2023-11-14-131400_coinbase/up.sql

+);
+
+-- Any old 'completed_transactions' will not be valid due to the removal of 'coinbase_block_height',
+-- so we drop and recreate the table.


why remove this?

base_layer/wallet/src/output_manager_service/storage/models.rs

Description --- Added normal one-sided and stealth one-sided coinbase transactions. Changes are: - Coinbases can now only be paid to a nominated wallet address directly. - The minotari miner and merge mining proxy will only start if a valid minotari wallet address has been supplied for the particular network. - The miner and merge mining proxy does not depend on the wallet anymore to construct the coinbase; it is done directly. - Corresponding gRPC method (`rpc GetCoinbase (GetCoinbaseRequest) returns (GetCoinbaseResponse)`) has been removed from the wallet. - A memory-based transactions key manager has been created for use by the miner and merge mining proxy as they do not need persistent storage of keys. This also ensures that new entropy for the generation of keys is created each time the miner or merge mining proxy is started effectively negating any collisions of private keys. - All unused code in the output manager and transaction manager have been removed. Motivation and Context --- See tari-project#5930 How Has This Been Tested? --- Unit tests Cucumber tests System-level tests - _Completed_ What process can a PR reviewer use to test or verify this change? --- - Code walk-through - Run cucumber `Scenario: Get Transaction Info` and review the log files for `WALLET_A` where the stealth one-sided coinbases will be imported and where payment is made to `WALLET_B`.   Breaking Changes --- - [ ] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [x] Other - Please specify  - Coinbases can now only be paid to a nominated wallet address directly. - Existing wallets need to be recovered into new wallets.

ghpbot-tari-project added the CR-too_long Changes Requested - Your PR is too long label Nov 18, 2023

hansieodendaal force-pushed the ho_one_sided_coinbase_approach_2 branch 2 times, most recently from 2bc4e36 to ee5729d Compare November 20, 2023 06:14

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Nov 20, 2023

hansieodendaal added 3 commits November 21, 2023 11:55

Add one-sided coinbase

615cc31

Added one-sided and stealth coinbase transactions. Coinbases can now only be paid to a nominated wallet address directly.

fixing cucumber tests

6c36242

fixing cucumber tests ..(2)

9bdbbc0

hansieodendaal changed the title ~~[wip] feat: add one-sided coinbase payments~~ feat: add one-sided coinbase payments Nov 21, 2023

hansieodendaal force-pushed the ho_one_sided_coinbase_approach_2 branch from 318285d to 12b1db3 Compare November 21, 2023 10:06

fixing cucumber tests..(3)

6a56f69

hansieodendaal force-pushed the ho_one_sided_coinbase_approach_2 branch from 12b1db3 to 6a56f69 Compare November 21, 2023 10:10

hansieodendaal requested review from SWvheerden, brianp and stringhandler November 21, 2023 10:31

hansieodendaal mentioned this pull request Nov 21, 2023

Remove GRPC via feature flag in wallet + 1-sided mining #5930

Closed

stringhandler changed the title ~~feat: add one-sided coinbase payments~~ feat!: add one-sided coinbase payments Nov 21, 2023

hansieodendaal commented Nov 21, 2023

View reviewed changes

review comments 1

c4d8dbd

hansieodendaal commented Nov 21, 2023

View reviewed changes

review comments 2

39a75da

hansieodendaal commented Nov 22, 2023

View reviewed changes

SWvheerden reviewed Nov 22, 2023

View reviewed changes

hansieodendaal added 3 commits November 22, 2023 15:18

review comments 3

32f9d00

review comments 4

1bdcb49

fix test

2136543

SWvheerden approved these changes Nov 23, 2023

View reviewed changes

ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 23, 2023

SWvheerden merged commit 89b19f6 into tari-project:development Nov 23, 2023
14 checks passed

hansieodendaal deleted the ho_one_sided_coinbase_approach_2 branch November 23, 2023 08:20

This was referenced Nov 24, 2023

[wip] feat: add one-sided coinbase payments - approach 1 #5937

Closed

transaction scanning not showing correctly #5986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add one-sided coinbase payments #5967

feat!: add one-sided coinbase payments #5967

hansieodendaal commented Nov 18, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

stringhandler commented Nov 21, 2023

hansieodendaal left a comment

hansieodendaal left a comment

hansieodendaal left a comment •

edited

Loading

SWvheerden left a comment

SWvheerden Nov 22, 2023

hansieodendaal Nov 22, 2023

SWvheerden Nov 22, 2023

hansieodendaal Nov 22, 2023 •

edited

Loading

SWvheerden Nov 22, 2023

hansieodendaal Nov 22, 2023

		@@ -0,0 +1,69 @@
		-- Any old 'outputs' will not be valid due to the removal of 'coinbase_block_height',

feat!: add one-sided coinbase payments #5967

feat!: add one-sided coinbase payments #5967

Conversation

hansieodendaal commented Nov 18, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

github-actions bot commented Nov 20, 2023 • edited Loading

Test Results (CI)

github-actions bot commented Nov 20, 2023 • edited Loading

Test Results (Integration tests)

stringhandler commented Nov 21, 2023

hansieodendaal left a comment

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

hansieodendaal left a comment • edited Loading

Choose a reason for hiding this comment

SWvheerden left a comment

Choose a reason for hiding this comment

SWvheerden Nov 22, 2023

Choose a reason for hiding this comment

hansieodendaal Nov 22, 2023

Choose a reason for hiding this comment

SWvheerden Nov 22, 2023

Choose a reason for hiding this comment

hansieodendaal Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

SWvheerden Nov 22, 2023

Choose a reason for hiding this comment

hansieodendaal Nov 22, 2023

Choose a reason for hiding this comment

hansieodendaal commented Nov 18, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

github-actions bot commented Nov 20, 2023 •

edited

Loading

hansieodendaal left a comment •

edited

Loading

hansieodendaal Nov 22, 2023 •

edited

Loading