#184 unconfirmed balance pr #191

dan-da · 2024-09-27T00:22:18Z

closes #184.

Implements unconfirmed balance feature for rust api, rpc-api, neptune-cli, and neptune-dashboard.

neptune-cli usage:

(after node mines 2 blocks and sends 5 coins to another wallet.)

$ neptune-cli --server-addr synced-balance
200

$ neptune-cli synced-balance-unconfirmed
195

neptune-dashboard display:

The unconfirmed balance is calculated as:

synced_balance - sum(mempool_spent_owned_utxos) + sum(mempool_unspent_owned_utxos).

(using checked_sub() and safe_add() NeptuneCoins methods).

Implementation:

Tthe mempool returns one or more MempoolEvents from every fn that mutates the mempool. All mutating calls must be performed though GlobalState methods which catch the events and forward them to WalletState::handle_mempool_events(). In this way, the wallet is able to learn when Tx are added or removed from the mempool and scan for inputs or outputs that are owned by the wallet. Matching utxos are stored in a map keyed by Tx hash, and are removed immediately when corresponding tx is removed from mempool. Importantly, this impl makes the mempool and wallet updates atomic, so they are always in sync.

note: my first impl used tokio broadcast channels to notify the wallet of mempool events, however it was not atomic and required a separate wallet task to listen for mempool events. The present solution is simpler and better.

a unit test is included that verifies confirmed and unconfirmed balances are both correct after mining a block to self and then sending 5 coins to another wallet. It further verifies that the unconfirmed balance changes to match confirmed balance after the mempool is cleared.

This PR adds rpc synced_balance_unconfirmed() and neptune-cli command synced-balance-unconfirmed.

Sword-Smith · 2024-10-16T14:03:05Z

src/locks/tokio/atomic_rw.rs

+    /// Attempt to acquire write lock immediately.
+    ///
+    /// If the lock cannot be acquired without waiting, an error is returned.
+    pub fn try_lock_guard_mut(&mut self) -> Result<AtomicRwWriteGuard<T>, TryLockError> {
+        self.try_acquire_write_cb();
+        let guard = self.inner.try_write()?;
+        Ok(AtomicRwWriteGuard::new(guard, &self.lock_callback_info))
+    }
+


Does not seem to be used in repository. Please use pub(crate) to spot such unused functions.

Sword-Smith · 2024-10-16T14:33:02Z

src/models/state/mempool.rs

+#[derive(Debug, Clone)]
+pub enum MempoolEvent {
+    /// a transaction was added to the mempool
+    AddTx(Transaction),
+
+    /// a transaction was removed from the mempool
+    RemoveTx(Transaction),
+
+    /// the mutator-set of a transaction was updated in the mempool.
+    ///
+    /// (Digest of Tx before update, Tx after mutator-set updated)
+    UpdateTxMutatorSet(Digest, Transaction),
+}


Very nice data structure!

Sword-Smith · 2024-10-16T14:40:29Z

src/models/state/mempool.rs

+/// Mempool updates must go through GlobalState so that it can
+/// forward mempool events to the wallet in atomic fashion.


A note on this comment:

In my mental model, there are multiple components to neptune-core: wallet, mempool, archival state, light state. The components should be separate and not be strongly linked, except through methods in GlobalState that may require the use of multiple components (as is for example the case with GlobalState::get_balance_history that reads both from the light_state and from the wallet_state. In other words: Mempool should not directly refer to the wallet, as the returned MempoolEvents could be used by any subscriber, such as e.g. a fancy block explorer that plays a sound for each new transaction in the mempool.

right. The point of MempoolEvent is to allow any type to be a mempool listener and to avoid linking WalletState and Mempool directly.

The comment is just to provide some app context for anyone reading mempool code and so they don't make mempool modifying methods pub in the future, which would allow bypassing GlobalState wrapper methods.

Sword-Smith · 2024-10-16T14:43:21Z

src/models/state/mempool.rs

-            self.queue.remove(&transaction_id);
-            debug_assert_eq!(self.tx_dictionary.len(), self.queue.len());
-            return rv;
+    pub(super) fn remove(&mut self, transaction_id: Digest) -> Result<Option<MempoolEvent>> {


Please don't add Result to the return type of all functions. Only add it if there's a good reason to.

The style that we are following in neptune-core is that problems that the user can trigger should result in an Error type, whereas problems that are the result of bad programming (by us) should immediately panic and give us a stacktrace for easier debugging.

Sword-Smith · 2024-10-16T14:44:04Z

src/models/state/mempool.rs

@@ -255,36 +294,42 @@ impl Mempool {
    ///
    /// Computes in θ(lg N)
    #[allow(dead_code)]
-    pub fn pop_max(&mut self) -> Option<(Transaction, FeeDensity)> {
+    fn pop_max(&mut self) -> Result<Option<(MempoolEvent, FeeDensity)>> {


Don't use Result here.

Sword-Smith · 2024-10-16T15:21:45Z

src/models/state/wallet/wallet_state.rs

+            let send_amt = NeptuneCoins::new(5);
+
+            // mine a block to our wallet.  we should have 100 coins after.
+            let tip_digest = mine_block_to_wallet(&mut global_state_lock).await?.hash();


Very expensive operation. Make sure to make this function call produce a deterministic Triton VM claim, so we can reuse the proofs that are used in tests.

Sword-Smith · 2024-10-16T15:24:09Z

src/models/state/wallet/wallet_state.rs

+                assert_eq!(
+                    gs.wallet_state
+                        .confirmed_balance(tip_digest, Timestamp::now())
+                        .await,
+                    coinbase_amt
+                );
+                assert_eq!(
+                    gs.wallet_state
+                        .unconfirmed_balance(tip_digest, Timestamp::now())
+                        .await,
+                    coinbase_amt
+                );


When using assert_eq, I always put the expected value first. I'd prefer if we stick to that convention.

Sword-Smith · 2024-10-16T15:25:00Z

src/models/state/wallet/wallet_state.rs

+        #[traced_test]
+        #[tokio::test]
+        async fn confirmed_and_unconfirmed_balance() -> Result<()> {
+            let mut rng = thread_rng();


You can use StdRng::seed_from_u64(\<some_number\>) here instead.

Sword-Smith · 2024-10-16T15:25:38Z

src/models/state/wallet/wallet_state.rs

+            // clear the mempool, which drops our unconfirmed tx.
+            global_state_lock
+                .lock_guard_mut()
+                .await
+                .mempool_clear()
+                .await?;
+
+            // verify that wallet's unconfirmed balance is 100 again.
+            assert_eq!(
+                global_state_lock
+                    .lock_guard()
+                    .await
+                    .wallet_state
+                    .unconfirmed_balance(tip_digest, Timestamp::now())
+                    .await,
+                coinbase_amt
+            );
+


Very thorough test. Love it!

Sword-Smith · 2024-10-16T15:27:55Z

src/tests/shared.rs

+// presently in the mempool.  The coinbase will go to our own wallet.
+//
+// the stored block does NOT have valid proof-of-work.
+pub async fn mine_block_to_wallet(global_state_lock: &mut GlobalStateLock) -> Result<Block> {


Nice convenience function. Please change to pub(crate) and drop the Result. Also: If you let this function take an explicit timestamp, then the transaction and block proofs can be reused across test runs if the rest of the setup is also deterministic.

wallet subscribes to mempool via tokio broadcast to track owned utxos. Changes: * add spent_utxos, unspent_utxos lists to WalletState struct * add WalletState::handle_mempool_event() * add test confirmed_and_unconfirmed_balance() * add Mempool::event_channel (tokio broadcast channel) * Mempool faillible mutation methods return Result() * Mempool mutable methods broadcast MempoolEvent * add tests::shared::mine_block_to_wallet() * lib.rs: spawn wallet task for listening to mempool events and dispatch to WalletState for handling. * add locks::tokio::AtomicRw::try_lock_guard_mut()

removes the mempool broadcast channel and wallet listener task. Instead all mempool mutations go through GlobalState methods which inform wallet of the changes. This makes changes atomic over mempool+wallet so they are always in sync. Changes: * remove Mempool::event_channel * Mempool &mut methods only callable by super * Mempool &mut methods return MempoolEvent(s) * add MempoolEvent::UpdateTxMutatorSet. (unused) * add GlobalState methods: mempool_clear, mempool_insert, mempool_prune_stale_transactions * remove spawn_wallet_task from lib.rs * add/improve doc-comments

dan-da force-pushed the danda/184_unconfirmed_balance_pr branch from da50084 to 82a9fc7 Compare October 15, 2024 18:37

Sword-Smith self-requested a review October 16, 2024 14:00

Sword-Smith requested changes Oct 16, 2024

View reviewed changes

dan-da added 3 commits October 16, 2024 21:54

fix: make confirmed_and_unconfirmed_balance deterministic

793aa63

dan-da force-pushed the danda/184_unconfirmed_balance_pr branch from 26ea666 to 793aa63 Compare October 16, 2024 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#184 unconfirmed balance pr #191

#184 unconfirmed balance pr #191

dan-da commented Sep 27, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

dan-da Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

Sword-Smith Oct 16, 2024

		/// Mempool updates must go through GlobalState so that it can
		/// forward mempool events to the wallet in atomic fashion.

#184 unconfirmed balance pr #191

Are you sure you want to change the base?

#184 unconfirmed balance pr #191

Conversation

dan-da commented Sep 27, 2024

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