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

Write to disk all UTXO notifications that come from own client #137

Closed
Sword-Smith opened this issue Apr 16, 2024 · 2 comments
Closed

Write to disk all UTXO notifications that come from own client #137

Sword-Smith opened this issue Apr 16, 2024 · 2 comments
Assignees
@dan-da
Labels
enhancement New feature or request good first issue Good for newcomers loss-of-funds any problem that could lead to user losing funds

Comments

@Sword-Smith
Copy link
Member

As I realized when discussing #136 we don't store UTXO notifications to disks. This means that if a client creates a transaction with a change UTXO and then shuts down before this transaction is included in a block, the change UTXO is not discovered by the client. #5 aims to address this issue in a fully recoverable fool proof way, but I think we should also just store the UTXO notifications to disk, iff they are worthy of storing to disk, that is -- this could be that they have a sufficiently high credibility, or, preferably just that they originate from the running instance, i.e. are of type UtxoNotifier::OwnMiner, UtxoNotifier::Cli, or UtxoNotifier::Myself.
@aszepieniec aszepieniec added enhancement New feature or request good first issue Good for newcomers labels Apr 16, 2024
@dan-da
Copy link
Collaborator

dan-da commented Apr 16, 2024

yeah, so here I am asking:

  1. why a change UTXO is treated differently from any other incoming UTXO
  2. why a change UTXO is treated differently from a UTXO that I am sending to my own wallet address, eg if splitting an existing wallet UTXO.

It seems we have two different ways of processing incoming UTXOs: expected_utxos and announcements. I presume there is a good reason, but I'd like to have a more thorough understanding before making any changes, especially as the asymmetry highlighted in (2) bothers me.

And generally it just seems simpler and easier to maintain to have a single mechanism.

@dan-da dan-da mentioned this issue Aug 1, 2024
Closed
dan-da added a commit that referenced this issue Aug 19, 2024
@dan-da
feat: persist ExpectedUtxo to disk (db)
d2f87f4 
closes #172, #137.

fixes loss-of-funds scenario.

changes:
 * add RustyWalletDatabase::expected_utxos (DbtVec)
 * remove UtxoNotificationPool
 * remove UtxoNotifier::PeerUnsigned
 * add WalletState methods:
    add_expected_utxo(), scan_for_expected_utxos()
 * disables timer call to prune_stale_expected_utxos() for now.
 + remove cli args:
    max_utxo_notification_size,
    max_unconfirmed_utxo_notification_count_per_peer
 * mod utxo_notification_pool --> expected_utxo
 * derive Hash for Timestamp

tests:
 * adapt existing tests to changes
 * move tests from utxo_notification_pool into wallet_state
 * adds regression tests for issue #172 that verify:
    1.  expected_utxo are persisted if db is written to disk.
    2.  expected_utxo are not persisted if db is not written to disk.
dan-da added a commit that referenced this issue Aug 19, 2024
@dan-da
feat: persist ExpectedUtxo to disk (db)
4abd0b1 
closes #172, #137.

fixes loss-of-funds scenario.

changes:
 * add RustyWalletDatabase::expected_utxos (DbtVec)
 * remove UtxoNotificationPool
 * remove UtxoNotifier::PeerUnsigned
 * add WalletState methods:
    add_expected_utxo(), scan_for_expected_utxos()
 * disables timer call to prune_stale_expected_utxos() for now.
 + remove cli args:
    max_utxo_notification_size,
    max_unconfirmed_utxo_notification_count_per_peer
 * mod utxo_notification_pool --> expected_utxo
 * derive Hash for Timestamp

tests:
 * adapt existing tests to changes
 * move tests from utxo_notification_pool into wallet_state
 * adds regression tests for issue #172 that verify:
    1.  expected_utxo are persisted if db is written to disk.
    2.  expected_utxo are not persisted if db is not written to disk.
@dan-da dan-da mentioned this issue Aug 19, 2024
Merged
dan-da added a commit that referenced this issue Aug 19, 2024
@dan-da
feat: persist ExpectedUtxo to disk (db)
a6eb73b 
closes #172, #137.

fixes loss-of-funds scenario.

changes:
 * add RustyWalletDatabase::expected_utxos (DbtVec)
 * remove UtxoNotificationPool
 * remove UtxoNotifier::PeerUnsigned
 * add WalletState methods:
    add_expected_utxo(), scan_for_expected_utxos()
 * disables timer call to prune_stale_expected_utxos() for now.
 + remove cli args:
    max_utxo_notification_size,
    max_unconfirmed_utxo_notification_count_per_peer
 * mod utxo_notification_pool --> expected_utxo
 * derive Hash for Timestamp

tests:
 * adapt existing tests to changes
 * move tests from utxo_notification_pool into wallet_state
 * adds regression tests for issue #172 that verify:
    1.  expected_utxo are persisted if db is written to disk.
    2.  expected_utxo are not persisted if db is not written to disk.
dan-da added a commit that referenced this issue Aug 19, 2024
@dan-da
feat: persist ExpectedUtxo to disk (db)
e908aba 
closes #172, #137.

fixes loss-of-funds scenario.

changes:
 * add RustyWalletDatabase::expected_utxos (DbtVec)
 * remove UtxoNotificationPool
 * remove UtxoNotifier::PeerUnsigned
 * add WalletState methods:
    add_expected_utxo(), scan_for_expected_utxos()
 * disables timer call to prune_stale_expected_utxos() for now.
 + remove cli args:
    max_utxo_notification_size,
    max_unconfirmed_utxo_notification_count_per_peer
 * mod utxo_notification_pool --> expected_utxo
 * derive Hash for Timestamp

tests:
 * adapt existing tests to changes
 * move tests from utxo_notification_pool into wallet_state
 * adds regression tests for issue #172 that verify:
    1.  expected_utxo are persisted if db is written to disk.
    2.  expected_utxo are not persisted if db is not written to disk.
dan-da added a commit that referenced this issue Aug 22, 2024
@dan-da
feat: persist ExpectedUtxo to disk (db)
232e852 
closes #172, #137.

fixes loss-of-funds scenario.

changes:
 * add RustyWalletDatabase::expected_utxos (DbtVec)
 * remove UtxoNotificationPool
 * remove UtxoNotifier::PeerUnsigned
 * add WalletState methods:
    add_expected_utxo(), scan_for_expected_utxos()
 * disables timer call to prune_stale_expected_utxos() for now.
 + remove cli args:
    max_utxo_notification_size,
    max_unconfirmed_utxo_notification_count_per_peer
 * mod utxo_notification_pool --> expected_utxo
 * derive Hash for Timestamp

tests:
 * adapt existing tests to changes
 * move tests from utxo_notification_pool into wallet_state
 * adds regression tests for issue #172 that verify:
    1.  expected_utxo are persisted if db is written to disk.
    2.  expected_utxo are not persisted if db is not written to disk.
@dan-da
Copy link
Collaborator

dan-da commented Sep 10, 2024

closed by 232e852

@dan-da dan-da closed this as completed Sep 10, 2024
@dan-da dan-da added the loss-of-funds any problem that could lead to user losing funds label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dan-da dan-da

Labels
enhancement New feature or request good first issue Good for newcomers loss-of-funds any problem that could lead to user losing funds
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@aszepieniec @dan-da @Sword-Smith