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

Add safe flag to listunspent result #9830

Merged
merged 2 commits into from
Mar 13, 2017
Merged

Add safe flag to listunspent result #9830

merged 2 commits into from
Mar 13, 2017

Conversation

NicolasDorier
Copy link
Contributor

This flag is useful to know if a particular UTXO will be considered by the wallet as part of fundrawtransaction of sendtoaddress.

@gmaxwell
Copy link
Contributor

I think trusted may be a bad name for this -- sure its the internal name but that doesn't mean it's good. :)

@NicolasDorier
Copy link
Contributor Author

what about "safe" ?

@laanwj
Copy link
Member

laanwj commented Feb 23, 2017 

(bool) Whether this coin is safe to spend (eligible to be spent by fundrawtransaction or sendtoaddress)

Call it 'eligible' maybe?

@jonasschnelli
Copy link
Contributor

Concept ACK.
Related: I wasn't really aware that fundraw with the watch-only option can't spend/select 0-conf watch-onlys. I think we should fix this.

@NicolasDorier NicolasDorier force-pushed the listunspenttrusted branch 2 times, most recently from 8d235bb to fe6cbed Compare February 23, 2017 12:55
@NicolasDorier
Copy link
Contributor Author

Fixed to 'eligible', it looks indeed better.

For my work on hdwatchonly, I kind of fixed it on 358ec69 .
This is only for hdwatchonly though. I think not all watchonly might be trusted.

But definitively, hdwatchonly generated address are trusted.

@ryanofsky
Copy link
Contributor

I think it would be bad to expose a new txout "eligible" attribute from listunspent which is 99% but not 100% identical to the "unsafe" attribute just exposed in 52dde66 (new in .14 release).

I think the best thing would be to decide whether the name "safe" or "eligible" is better and use that name consistently. CWallet::AvailableCoins could be modified to return whether each coin is safe/eligible, either by adding a new fIsSafe/fIsEligible field to COutput, or by just changing the function arguments. The CWallet::AvailableCoins fOnlyConfirmed argument could also be eliminated, or renamed to fOnlySafe/fOnlyEligible.

@NicolasDorier
Copy link
Contributor Author

@ryanofsky I think it is rather different purpose. include_unsafe act as a filter, here I need an attribute to differenciate whose which will be selected by fundrawtransaction and those which will not.

@ryanofsky
Copy link
Contributor

Right, include_unsafe is a filter on the safe/eligible boolean that you want to return. Maybe my suggestion for how to unify the two implementations is unclear? Let me know if the changes I was suggesting for AvailableCoins don't make sense. I'd also be happy to post a PR.

@NicolasDorier
Copy link
Contributor Author

ah got it, you want both to be the same rather than slightly different ? I would be happy to see a PR about it, the difference is indeed confusing.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 23, 2017
@ryanofsky
Add trusted flag to listunspent result
f4cace4 
Based on commit fe6cbed Add eligible flag to listunspent result (NicolasDorier)
from bitcoin#9830.
@ryanofsky
Copy link
Contributor

@NicolasDorier I posted a possible implementation in
ryanofsky@f4cace4 based on your PR. I used "safe" instead of eligible here because it seemed more natural to me, but I think either name would be ok. Maybe take a look, and feel free to incorporate any of these changes.

@ryanofsky
Copy link
Contributor

ryanofsky commented Feb 23, 2017 •
edited
Loading

This branch with the same change is a little better: https://github.com/ryanofsky/bitcoin/commits/pr/safelist

It splits it up into a refactoring commit ad89a07, and a commit updating the RPC 5c08fcb.

@NicolasDorier
Copy link
Contributor Author

@ryanofsky I like your branch. Will update this PR.

@luke-jr
Copy link
Member

luke-jr commented Feb 25, 2017

Suggest refactoring by adding a CWalletTx::IsSafe also.

@NicolasDorier NicolasDorier changed the title Add trusted flag to listunspent result Add safe flag to listunspent result Feb 26, 2017
@NicolasDorier
Copy link
Contributor Author

As @ryanofsky advice, I renamed the flag to "safe". I added his commit which add a safe flag to the COutput type and rename the parameter in CWallet::AvailableCoins.

I then renamed CWallet::IsTrusted to CWallet::IsSafe which more properly reflect the meaning of the function.

@ryanofsky
Copy link
Contributor

utACK 037d2876a00cb6a3a01de9beb604b6bfa8d2601c.

Personally I would prefer not to rename IsTrusted function to IsSafe because the trusted and safe conditions are not identical. I think luke-jr's suggestion to add a new IsSafe method that takes a wallet tx instead of a plain tx and computes the real fSafe value would be better.

@NicolasDorier
Copy link
Contributor Author

@ryanofsky fair enough, I removed the renaming. (it was the last commit)
Can you re-ACK?

I think to deal with deeper refactoring in another PR. This one is only for exposing more information.

@ryanofsky
Copy link
Contributor

utACK 23cbc366a7c3633e694286a693a4223de02b95ad

@ryanofsky
Copy link
Contributor

Needs rebase because of #9643

ryanofsky and others added 2 commits March 10, 2017 05:11
@ryanofsky @NicolasDorier
Add COutput::fSafe member for safe handling of unconfirmed outputs
af61d9f 
This exposes a value computed in CWallet::AvailableCoins so it can used for
other things, like inclusion in listunspent output.
@NicolasDorier
Add safe flag to listunspent result
dcf2112
@NicolasDorier
Copy link
Contributor Author

rebased

@ryanofsky
Copy link
Contributor

utACK dcf2112, confirmed no changes in rebase.

@jonasschnelli
Copy link
Contributor

utACK af61d9f

1 similar comment
@btcdrak
Copy link
Contributor

btcdrak commented Mar 12, 2017

utACK af61d9f

@TheBlueMatt
Copy link
Contributor

utACK dcf2112

@laanwj laanwj merged commit dcf2112 into bitcoin:master Mar 13, 2017
laanwj added a commit that referenced this pull request Mar 13, 2017
@laanwj
Merge #9830: Add safe flag to listunspent result
afcd7c0 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 21, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9830: Add safe flag to listunspent result
af2831b 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9830: Add safe flag to listunspent result
bcbb096 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 26, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9830: Add safe flag to listunspent result
1b2aad1 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 26, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9830: Add safe flag to listunspent result
ee2048a 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Mar 9, 2019
@laanwj @UdjinM6
Merge bitcoin#9830: Add safe flag to listunspent result
b7a3c75 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 10, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9830: Add safe flag to listunspent result
b442686 
dcf2112 Add safe flag to listunspent result (NicolasDorier)
af61d9f Add COutput::fSafe member for safe handling of unconfirmed outputs (Russell Yanofsky)

Tree-SHA512: 311edb6fa8075b3ede5b24cb8c6e5d133ccd8ac9ecafea07b604ffa812ee4f071337e31695e662d8573590a0460af20aaaeb39d49c9ea87924449ea50bdfb0b3
@random-zebra random-zebra mentioned this pull request Apr 28, 2021
Merged
1 task
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jun 9, 2021
@random-zebra
Merge #2351: [Wallet] Laggard wallet-related backports from btc 0.15
2d50b6e 
d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider)
3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider)
dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli)
20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky)
e411b70 [wallet] Fix leak in CDB constructor (João Barbosa)
f15aeea Change getmininginfo errors field to warnings (Andrew Chow)
c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra)
1d966ce Add warnings field to getblockchaininfo (Andrew Chow)
ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra)
e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra)
e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra)
2188c3e Move some static functions out of wallet.h/cpp (random-zebra)
f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra)
8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra)
900bbfa Reject invalid wallet files (João Barbosa)
a1f4e2a Reject duplicate wallet filenames (random-zebra)
ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky)
ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra)
37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra)
3955ee9 [Doc] Update release notes (random-zebra)
4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery)
1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery)
fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery)
cf4a90b Remove factor of 3 from definition of dust. (random-zebra)
a1c56fd [Policy] Introduce -dustrelayfee (random-zebra)
9fb29cc [Doc] Update multiwallet release notes (random-zebra)
379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra)
808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra)
f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery)
2e02006 Rename -usewallet to -rpcwallet (Alex Morcos)
a64440b Select wallet based on the given endpoint (Jonas Schnelli)
5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra)
b0fe92f Fix test_pivx circular dependency issue (random-zebra)
6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli)
7dd3916 Register wallet endpoint (Jonas Schnelli)
5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos)
41a7335 Remove unused variables (practicalswift)
5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky)
e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra)
54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra)
494ba64 [test] Add test for getmemoryinfo (random-zebra)
2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)
7d977ac Remove unused C++ code not covered by unit tests (random-zebra)
60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar)
3633d75 Initialize nRelockTime (Patrick Strateman)
3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra)
f219be9 Add safe flag to listunspent result (NicolasDorier)
0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra)
75c8c6d Disallow copy of CReserveKeys (Gregory Sanders)
8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra)

Pull request description:

  I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us.
  I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files).

  This changeset is based on top of
  - [x] #2337

  as it keeps going with the multiwallet implementation, adding:
  - RPC endpoint support
  - `listwallets` RPC
  - wallet name in `getwalletinfo` RPC
  - `wallet_multiwallet.py` functional test

  As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code.

  List of upstream PRs backported/adapted:

  - bitcoin#9906  : Disallow copy constructor CReserveKeys
  - bitcoin#9830  : Add safe flag to listunspent result
  - bitcoin#9993  : Initialize nRelockTime
  - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value
  - bitcoin#10075 : Remove unused C++ code not covered by unit tests
  - bitcoin#10257 : Add test for getmemoryinfo
  - bitcoin#10295 : Move some WalletModel functions into CWallet
  - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings
  - bitcoin#10522 : Remove unused variables
  - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet
  - bitcoin#10849 : Multiwallet: simplest endpoint support
  - bitcoin#9380  : Separate different uses of minimum fees (only dustRelayFee)
  - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit)
  - bitcoin#10883 : Rename -usewallet to -rpcwallet
  - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests
  - bitcoin#10893 : Avoid running multiwallet.py twice
  - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets
  - bitcoin#10931 : Fix misleading "method not found" multiwallet errors
  - bitcoin#10885 : Reject invalid wallets
  - bitcoin#11022 : Basic keypool topup
  - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp
  - bitcoin#11310 : Test listwallets RPC
  - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs
  - bitcoin#11492 : Fix leak in CDB constructor
  - bitcoin#11476 : Avoid opening copied wallet databases simultaneously
  - bitcoin#11590 : always show help-line of wallet encryption calls
  - bitcoin#11289 : Add wallet backup text to import* and add* RPCs

ACKs for top commit:
  furszy:
    re-re-utACK d5526bd.
  Fuzzbawls:
    ACK d5526bd

Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@NicolasDorier @gmaxwell @laanwj @jonasschnelli @ryanofsky @luke-jr @btcdrak @TheBlueMatt