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.

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

Allow UTXO locks to be written to wallet DB #23065

Merged
merged 5 commits into from
Sep 26, 2021
Merged

Allow UTXO locks to be written to wallet DB #23065

merged 5 commits into from
Sep 26, 2021

Conversation

meshcollider
Copy link
Contributor

@meshcollider meshcollider commented Sep 22, 2021

Addresses and closes #22368

As per that issue (and its predecessor #14907), there seems to be some interest in allowing unspent outputs to be locked persistently. This PR does so by adding a flag to lockunspent to store the change in the wallet database. Defaults to false, so there is no change in default behaviour.

Edit: GUI commit changes default behaviour. UTXOs locked/unlocked via the GUI are now persistent.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Sep 22, 2021

Concept ACK

Thanks a lot for improving privacy and working on this issue. Will test it in few minutes.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 22, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22929 (wallet: Automatically add receiving destinations to the address book by S3RK)
  • #22693 (RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr)
  • #20591 (wallet, bugfix: fix ComputeTimeSmart function during rescanning process. by BitcoinTsunami)
  • #20205 (wallet: Properly support a wallet id by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ghost
Copy link

ghost commented Sep 22, 2021

Tested on Pop!_OS and everything works as expected.

Steps that I followed for testing:

Check default behavior ✅

bitcoin-cli -rpcwallet=W1 listunspent

[
  {
    "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
    "vout": 0,
    "address": "bcrt1qa8xy0hlvlsr66yvv80h3qxp9stq68aef6pzvjt",
    "label": "",
    "scriptPubKey": "0014e9cc47dfecfc07ad118c3bef10182582c1a3f729",
    "amount": 0.00010000,
    "confirmations": 1003,
    "spendable": true,
    "solvable": true,
    "desc": "wpkh([0ab0c859/0'/0'/2']03c65a6e4a66a4a6b4af981c5e81632bb2d7336bc75d7ccf83e055006a2693050f)#6z2c6spl",
    "safe": true
  }
]

$ bitcoin-cli -rpcwallet=W1 lockunspent false "[{\"txid\":\"752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff\",\"vout\":0}]"
true

$ bitcoin-cli -rpcwallet=W1 listlockunspent
[
  {
    "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
    "vout": 0
  }
]

$ bitcoin-cli stop
Bitcoin Core stopping

$ bitcoind

$ bitcoin-cli -rpcwallet=W1 listlockunspent
[
]

Use the new argument to save locked UTXO in database ✅

$ bitcoin-cli -rpcwallet=W1 lockunspent false "[{\"txid\":\"752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff\",\"vout\":0}]" true
true

$ bitcoin-cli -rpcwallet=W1 listlockunspent
[
  {
    "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
    "vout": 0
  }
]

$ bitcoin-cli stop
Bitcoin Core stopping

$ bitcoind

$ bitcoin-cli -rpcwallet=W1 listlockunspent
[
  {
    "txid": "752871b9f6aeacbd1d60a0154aabd098cd7b172b74928f9af0a6104af0dbf9ff",
    "vout": 0
  }
]

Errors ✅

  1. Trying to unlock a UTXO which isn't locked
$ bitcoin-cli -rpcwallet=W1 lockunspent true "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]" true

error code: -8
error message:
Invalid parameter, expected locked output
  1. Trying to lock a UTXO which is already locked
$ bitcoin-cli -rpcwallet=Wallet1 lockunspent false "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]" true

error code: -8
error message:
Invalid parameter, output already locked

ℹ️ It also works for descriptor wallets

⚠️ There are some issues with GUI but I guess they can be addressed in https://github.com/bitcoin-core/gui

  • No option to use this new argument and save locked coins in db
  • If UTXO is locked in db is bitcoin-cli its greyed out and there is an option to unlock it. This unlocked state won't be saved in db and will remain until GUI is running.

@meshcollider Thanks for adding this option :) It will improve privacy and few projects were also dependent on this feature. Example: https://github.com/p2pderivatives/p2pderivatives-client#known-limitations

@kristapsk
Copy link
Contributor

Concept ACK

@meshcollider
Copy link
Contributor Author

Thanks, addressed both review suggestions.

Happy to make GUI changes persistent too, it would be a very simple change. Can discuss if it is a useful change.

@kristapsk
Copy link
Contributor

Happy to make GUI changes persistent too, it would be a very simple change. Can discuss if it is a useful change.

I think it is. People who use manual coin control will likely manually selected specfic UTXOs for single tx and lock unspent for longer term. And longer term should survive bitcoin-qt restarts. At least that looks to me more intuitive. Also, from privacy perspective, it is better to have some unwanted UTXO locked longer than spend it accidentally.

@meshcollider
Copy link
Contributor Author

Sure, added persistent locking to the GUI now then 👍

@ghost
Copy link

ghost commented Sep 23, 2021

tACK 687d65f

Changes since last review:

  • GUI
  • Release notes
  • Comment in test
  • Example
Tried locking/unlocking UTXOs in GUI (No issues)

image

image

image

@ghost
Copy link

ghost commented Sep 23, 2021

wallet_address_types.py --descriptors | ✖ Failed | 25 s

Not sure if this error in CI is related to PR: https://github.com/bitcoin/bitcoin/pull/23065/checks?check_run_id=3682608670

@meshcollider
Copy link
Contributor Author

meshcollider commented Sep 23, 2021

@prayank23 Not sure if this error in CI is related to PR

Looks unrelated, it looks like some random windows socket issue:

OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

reACK 2d3ed88

Changes since last review:

Argument name changed which can be used to save the state of UTXO lock in db as suggested in #23065 (comment)

-store_lock
+persistent

@kristapsk
Copy link
Contributor

kristapsk commented Sep 24, 2021

I think this needs also release note about existing behaviour change in the GUI.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

I have some questions though:

  • lock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?
  • unlock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?
  • maybe unlock should be sensible to the existing lock (persisted or not)?

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

In CWallet::AddToSpends we should remove persisted lock?

@meshcollider
Copy link
Contributor Author

lock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?

I don't think we should allow re-locking in any case, better to require explicitly unlocking then locking again.

unlock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?

I was thinking a lot about this while writing the PR. It does make sense to me that unlock would simply remove any locks, persistent or not. But I am not sure if there are some use cases that would benefit from the functionality - persistently lock a UTXO and then temporarily unlock it for a while for some reason? I'm happy to change this if people agree.

Good points about AddToSpends and the release notes, will fix.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Why not just always persist? What are the cons of that? If someone wants no locks then just call lockunspent true after load.

@laanwj
Copy link
Member

laanwj commented Sep 24, 2021

Why not just always persist? What are the cons of that? If someone wants no locks then just call lockunspent true after load.

Not sure about this. It's a much larger behavior change (downstream software probably relies on starting with a clean slate after restart), and if this behavior really turns out to be more popular it could always be done later.

@promag
Copy link
Member

promag commented Sep 24, 2021

Why not just always persist? What are the cons of that? If someone wants no locks then just call lockunspent true after load.

Not sure about this. It's a much larger behavior change (downstream software probably relies on starting with a clean slate after restart), and if this behavior really turns out to be more popular it could always be done later.

Not only restart, even loadwallet after unloading.

How about a startup flag like --persist-unspent-locks?

If we keep this approach then it needs a way to select persist=true in all calls that eventually lock coins, like fundrawtransaction lockUnspents=true. Suddenly there's a lot of options around.

I just think a (good) behavior change looks saner to reason about.

@laanwj
Copy link
Member

laanwj commented Sep 24, 2021

How about a startup flag like --persist-unspent-locks?

Please don't add another startup option that subtly changes behavior over all wallets 😄
I was actually fearing this when I looked at this PR, and was pleasantly surprised it's a flag per utxo.

@promag
Copy link
Member

promag commented Sep 24, 2021

@meshcollider I think you should mention that only manual locks can be persisted. Funding calls result in memory-only locks.

@korbanr korbanr mentioned this pull request Sep 24, 2021
Closed
@achow101
Copy link
Member

The GUI change needs a release note.

@korbanr korbanr mentioned this pull request Sep 24, 2021
Closed
@meshcollider
Copy link
Contributor Author

I'll leave any potential behaviour changes with fundrawtransaction/send RPCs and lock reasons for a follow-up PR, to keep this change simpler. Storing the reason should be easy, just using the value of the LOCKED_UTXO DB field which is currently just always 1.

test/functional/wallet_basic.py Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@@ -2260,22 +2260,36 @@ bool CWallet::DisplayAddress(const CTxDestination& dest)
return signer_spk_man->DisplayAddress(scriptPubKey, signer);
}

void CWallet::LockCoin(const COutPoint& output)
bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch)
{
AssertLockHeld(cs_wallet);
setLockedCoins.insert(output);
Copy link
Member

Choose a reason for hiding this comment

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

Re #23065 (comment)

lock(utxo, persistent=false) but utxo already has persisted lock - should delete from db?

I don't think we should allow re-locking in any case, better to require explicitly unlocking then locking again.

If you want to implement this breaking change then use the return value of insert() to know if the output was already locked and return false.

Otherwise, the lock can be persisted but not in memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, the lock can be persisted but not in memory.

Sorry, I'm not sure how this could happen. Upon thinking further, I think its fine to upgrade a memory-only lock to a persistent lock (this is useful if fundrawtransaction locked the spends and we want to persistently lock them afterward). But how could we end up with a persistent lock not in memory?

EDIT: I've updated the PR to allow persistently locking even if we already have a lock, to allow upgrading.

Copy link
Member

Choose a reason for hiding this comment

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

But how could we end up with a persistent lock not in memory?

Right, doesn't happen now that unlock clears from memory and db.

Copy link
Member

@promag promag Sep 25, 2021

Choose a reason for hiding this comment

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

But does it make sense to allow memory-only lock if it is already persisted?

EDIT: looks like this is not possible since lockunspent RPC checks if unspent is already locked.

@Pob1212
Copy link

Pob1212 commented Sep 25, 2021

gh pr checkout 23065

@achow101
Copy link
Member

ACK d96b000

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK d96b000

@ghost
Copy link

ghost commented Sep 26, 2021

Many changes since last review: https://github.com/bitcoin/bitcoin/compare/2d3ed88..d96b000, agree that changes about GUI should be mentioned in release notes and few things looked confusing in above discussion so tested again.

  1. GUI only : Lock/Unlock no issues.

  2. CLI:
    2.1 Errors: Invalid parameter, output already locked this is not printed in one case when when you try to lock UTXO which is already locked with new parameter set as true⚠️

    2.2 Locking UTXO without using new parameter works as expected and reset to unlocked on restart
    2.3 Locking UTXO with new parameter set as true works as expected. Locked state is saved in db and remains even after restart.

    2.4 Lock UTXO without new parameter. Unlock UTXO with new parameter set as true. Unlocked state remains after restart.
    2.5 Lock UTXO with new parameter (db). Unlock UTXO without new parameter. Unlocked state remains after restart ❌

  3. CLI + GUI:
    3.1 Lock UTXO with and without new parameter. Stop bitcoind. Launch bitcoin-qt and check if state remains as expected. No issues.

    3.2 Lock UTXO from CLI (with new param). Stop bitcoind. Unlock from GUI. Restart bitcoind. Check if unlocked state remains. No issues.
    3.3 Unlock UTXO from CLI (with new param). Stop bitcoind. Lock from GUI. Restart bitcoind. Check if locked state remains. No issues.

    3.4 Lock UTXO from CLI (without new param). Stop bitcoind. Lock from GUI. Restart bitcoind. Check if locked state remains. No issues.
    3.5 Unlock UTXO from CLI (without new param). Stop bitcoind and check state in bitcoin-qt. It remains locked ❌

2.5 and 3.5 doesn't look correct IMO. Also agree with @promag that very few people will be affected if this is made persistent by default in CLI and GUI. Infact people might start using this after this change.

@achow101
Copy link
Member

Lock UTXO with new parameter (db). Unlock UTXO without new parameter. Unlocked state remains after restart x

No? There's also a test for that case. Perhaps you are looking at a previous revision.

@meshcollider
Copy link
Contributor Author

meshcollider commented Sep 26, 2021

Thanks @prayank23 for your detailed testing!

2.1 Errors: Invalid parameter, output already locked this is not printed in one case when when you try to lock UTXO which is already locked with new parameter set as true⚠️

Yes, I decided it isn't worth the overhead of checking if the persistent lock exists in the database, if someone wants to lock the same output twice persistently then I don't see why that should error. It will, however, error if you try and lock un-persistently if the lock already exists -- this is to avoid confusion about downgrading a persistent lock to non-persistent (not allowed).

2.5 and 3.5 doesn't look correct IMO.

As @achow101 mentioned, this doesn't seem to be the case. I manually tested 3.5 and cannot replicate the issue -- the lock is definitely cleared persistently and does not remain locked for me. Could you check you built the latest version?

Copy link
Contributor

@lsilva01 lsilva01 left a comment

Choose a reason for hiding this comment

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

Tested ACK d96b000 on Ubuntu 20.04

2.5 and 3.5 doesn't look correct IMO.

I also cannot replicate the issues. The steps mentioned in 2.5 and 3.5 worked as expected in my tests.

@ghost
Copy link

ghost commented Sep 26, 2021

Steps that I did for 2.5:

$ bitcoind --version
Bitcoin Core version v22.99.0-d96b000e94d7

$ bitcoind

$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent
[
]

$ bitcoin-cli -rpcwallet=Wallet1 lockunspent false "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]" true

true

$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent

[
  {
    "txid": "f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7",
    "vout": 1
  }
]

$ bitcoin-cli -rpcwallet=Wallet1 lockunspent true "[{\"txid\":\"f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7\",\"vout\":1}]"

true

$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent

[
]

$ bitcoin-cli stop

$ bitcoind

$ bitcoin-cli -rpcwallet=Wallet1 listlockunspent

[
]
PR23065.mp4

@meshcollider
Copy link
Contributor Author

meshcollider commented Sep 26, 2021

Oh, apologies @prayank23 , I misread your comment. That behaviour is intended:

I was thinking a lot about this while writing the PR. It does make sense to me that unlock would simply remove any locks, persistent or not.

We discussed this briefly on IRC at the wallet meeting and decided this approach made the most sense.

(By the way, if you start bitcoind with the -daemon flag you don't need to use two terminal windows :) )

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

ACK d96b000

nit: I will be using persistence parameter set to true every time or GUI so 2.5 and 3.5 isn't an issue for me personally. Can be confusing for some users.

@meshcollider
Copy link
Contributor Author

@prayank23 Thanks! Happy to discuss changing the default after this is merged as a follow-up.

@laanwj laanwj merged commit 09cb5ec into bitcoin:master Sep 26, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2021
@ryanofsky ryanofsky mentioned this pull request Oct 6, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wallet: use database for locked coins
9 participants