Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ignore unconfirmed spent input #198

Closed
wants to merge 3 commits into from
Closed

ignore unconfirmed spent input #198

wants to merge 3 commits into from

Conversation

dan-da
Copy link
Collaborator

@dan-da dan-da commented Oct 4, 2024

Closes #189.

Extends #184.

Alternative to #197. (we should choose one or the other)

This PR provides a fix for the issue reported in #189. Unlike #197, it does not implement unconfirmed tx chaining.

from commit msg:

    input selection now ignores spent inputs from unconfirmed tx in the
    mempool.
    
    Also fixes an input selection bug where the balance check considers
    available funds (not timelocked) but the input utxo selection does not.
    
    Adds a test to verify that same input can no longer be spent twice.

@dan-da
feat: wallet unconfirmed balance.
98f8071 
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()
@dan-da
refactor: wallet updates now atomic with mempool
da50084 
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
fix: prevent spending same input twice.
9ec1de6 
closes Neptune-Crypto#189.

input selection now ignores spent inputs from unconfirmed tx in the
mempool.

Also fixes an input selection bug where the balance check considers
available funds (not timelocked) but the input utxo selection does not.

Adds a test to verify that same input can no longer be spent twice.
Sword-Smith
Sword-Smith reviewed Oct 11, 2024
View reviewed changes
src/models/state/mempool.rs
@@ -470,7 +529,7 @@ mod tests {
use super::*;

#[tokio::test]
pub async fn insert_then_get_then_remove_then_get() {
pub async fn insert_then_get_then_remove_then_get() -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the test does not return anything, then we get a nice stack trace whenever it fails. If the test returns some error, then the stack trace is not as useful, is my experience. For this reason, I prefer using unwrap or expect in tests, not the ? operator.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backtraces can be automatically displayed for anyhow errors via:

cargo.toml

anyhow = { version = "1.0", features = ["backtrace"] }

.cargo/config.toml

[env]
RUST_BACKTRACE="1"

With these enabled, when a test returns an anyhow::Error a backtrace is displayed. example:

---- rpc_server::rpc_server_tests::send_to_many_test stdout ----
Error: Do we get a stack trace?

Stack backtrace:
   0: anyhow::error::<impl anyhow::Error>::msg
             at /home/danda/.cargo/registry/src/index.crates.io-6f17d22bba15001f/anyhow-1.0.81/src/backtrace.rs:27:14
   1: anyhow::__private::format_err
             at /home/danda/.cargo/registry/src/index.crates.io-6f17d22bba15001f/anyhow-1.0.81/src/lib.rs:688:13
   2: neptune_core::rpc_server::rpc_server_tests::send_to_many_test::{{closure}}
             at ./src/rpc_server.rs:1399:9
<truncated>

I will add these changes to the PR.

I choose to avoid unwrap/expect whenever possible in code I author, but I agree it is nice to have backtraces for errors in tests (and elsewhere). Fortunately, its possible to do both.

Sword-Smith
Sword-Smith reviewed Oct 11, 2024
View reviewed changes
src/models/state/mempool.rs
Comment on lines +63 to +80
/// Represents a mempool state change.
///
/// For purpose of notifying interested parties
#[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),
}

#[derive(Debug, GetSize)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments:

  1. Our files are growing pretty big. Can we have a separate file for this datatype?
  2. The MempoolEvent should only contain a TransactionKernel. It doesn't need the transaction proof for anything, as the mempool already has that (where it's used for sharing transactions with peers and merging transactions when mining). The MempoolEvents received by the wallet only need the TransactionKernel.

@Sword-Smith
Copy link
Member

I just realized we have a great usecase for the MempoolEvent::UpdateTxMutatorSet variant: If we've performed the update of a transaction in our mempool, we should broadcast this transaction (as a transaction notification) to all peers since they might not have performed the update of the transaction proof.

Very cool! Just reveals that this is great design :)

@aszepieniec aszepieniec mentioned this pull request Oct 24, 2024
Open
13 tasks
@Sword-Smith
Copy link
Member

I'm merging this now :)

@Sword-Smith Sword-Smith self-requested a review October 24, 2024 12:18
Sword-Smith added a commit that referenced this pull request Oct 24, 2024
@Sword-Smith
Merge remote-tracking branch 'dan-da/danda/184_unconfirmed_balance_no…
9ba7ee9 
…_result'

Makes wallet aware of transactions in mempool such that it doesn't try
to spend the same UTXO twice if initiating a 2nd transaction before the
1st one has been mined.

Cf. #198.

Closes #189.

Co-authored-by: dan-da <[email protected]>
@Sword-Smith
Copy link
Member

Merged in 9ba7ee9

Sword-Smith added a commit that referenced this pull request Oct 24, 2024
@Sword-Smith @dan-da
Merge remote-tracking branch 'dan-da/danda/184_unconfirmed_balance_no…
c1e5d03 
…_result'

Makes wallet aware of transactions in mempool such that it doesn't try
to spend the same UTXO twice if initiating a 2nd transaction before the
1st one has been mined.

Cf. #198.

Closes #189.

Co-authored-by: dan-da <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sword-Smith Sword-Smith Sword-Smith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

create_transaction(), send(), should consider Tx in mempool.
2 participants
@dan-da @Sword-Smith