Add safe flag to listunspent result #9830

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

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

what about "safe" ?

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

Call it 'eligible' maybe?

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.

Fixed to 'eligible', it looks indeed better.

For my work on hdwatchonly, I kind of fixed it on https://github.com/bitcoin/bitcoin/pull/9728/commits/358ec69ae0a809db030a5a4e72133bc538026cbe . This is only for hdwatchonly though. I think not all watchonly might be trusted.

But definitively, hdwatchonly generated address are trusted.

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 52dde66770d833ee5e42e7c5fee610453ae3852a (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.

@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.

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.

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.

@NicolasDorier I posted a possible implementation in https://github.com/ryanofsky/bitcoin/commit/f4cace484cde19ee285bbb0260b51fc251665d1c 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.

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 ad89a07bb16fb13a7007eecfb03b7ec931d54751, and a commit updating the RPC 5c08fcb9277b40fe38d2582dc01d52ade3e2b1de.

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

Suggest refactoring by adding a CWalletTx::IsSafe also.

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.

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.

@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.

utACK 23cbc366a7c3633e694286a693a4223de02b95ad

Needs rebase because of #9643

rebased

utACK dcf2112de6ec5a7a5e97076d1ce826eb233a1042, confirmed no changes in rebase.

utACK af61d9f78bec62ff3688d88409a53df9ff5bc591

utACK af61d9f78bec62ff3688d88409a53df9ff5bc591

utACK dcf2112de6ec5a7a5e97076d1ce826eb233a1042