👍 hernanmarino approved a pull request: "cli: add validation to cli side commands besides when it's used with -rpcwallet"
(https://github.com/bitcoin/bitcoin/pull/26990)
re ACK 7f5fd1e6999b955dbb0b382bbfdce90a02e1cd8e Looks good to me now

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123258118)
I wrote it in that way before but changed it due a previous code review suggestion (previous [diff](https://github.com/bitcoin/bitcoin/compare/4343835a23290f2cd34404821494303cc75ce86e..5246cc1098e5409e986c1f215bc88eb44cec62f8)). I'm not fan of bool args neither but, at least here, I would be ok either way.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123260283)
yes, I didn't remove it just to make the diff smaller. This was already there.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123270988)
What are you referring with "mixed type groups"?

The `mixed` term is only used inside the `Groups` structure, and reflects a vector of `OutputGroup` that may contain positive and negative UTXOs.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123295722)
The "filter" term usage here in the comment is wrong. For what corresponds to this struct, the `all` term just refers to the combination of all inserted groups. Same as we do in the `CoinsResult::All` method. The filtering process, nor what is inserted (whether is filtered or not), is not responsibility of this struct.

It's just a container that keeps track of `Groups` by type and the caches the combination of all of them.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123305653)
for the `FilteredOutputGroups` map. `std::map` requires a comparator or a hash function.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123314932)
`GroupsByValueSign`?

I prefer the struct rather than having an ugly std::pair or a typedef:
`typedef std::pair<std::vector<OutputGroup>, std::vector<OutputGroup>> GroupsByValueSign`

💬 ryanofsky commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784)
In commit "consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts`" (97292dafb528d2a097f29e4c79a08cced49e5451)

Should this use emplace_back instead of push_back?

⚠️ da2ce7 opened an issue: "Coin Controll for Unconfirmed Outputs"
(https://github.com/bitcoin/bitcoin/issues/27190)
I would like to be able to select unconfined outputs in coin-control. For example allowing me to make a child-pays-for-parent transaction.

💬 MarcoFalke commented on pull request "wallet: ensure the wallet is unlocked when needed for rescanning":
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123340738)
This fails for me when running in valgrind:

```
node0 2023-02-28T19:50:00.090808Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/request.cpp:179] [parse] [rpc] ThreadRPCServer method=walletpassphrase user=__cookie__
node0 2023-02-28T19:50:08.966047Z (mocktime: 2023-03-30T19:35:09Z) [httpworker.1] [rpc/server.cpp:560] [RPCRunLater] [rpc] queue run of timer lockwallet(encrypted_wallet) in 1 seconds (using HTTP)
node0 2023-02-28T19:50:08.995457Z (mocktime: 2023-03-30T19:35:09Z) [ht
...

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1123341750)
What I learned is that windows/FAT systems have 2-second resolution but Linux/EXT4 etc have ms or ns resolution. I had a hard time determining the best method for testing when a file is flushed but I think you're right, using timestamps is just asking for intermittency. I think I wanted to avoid hashing files because it means opening the file while blockmanager might already have it open... but I think that's going to be the best way to test.

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123363865)
yeah

💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123369441)
I mean, it would be slower but could also be created on-demand:
```c++
Groups OutputGroups::All()
{
Groups ret;
for (const auto& [type, groups_vec] : groups_by_type) {
ret.positive_group.insert(ret.positive_group.begin(), groups_vec.positive_group.begin(), groups_vec.positive_group.end());
ret.mixed_group.insert(ret.mixed_group.begin(), groups_vec.mixed_group.begin(), groups_vec.mixed_group.end());
}
return ret;
}
```

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1123380652)
ok trying another approach where we ALWAYS pause, to ensure incremental timestamps. But on windows we have to pause for 2 seconds. All other filesystems we only need to wait for 1 ms.

💬 pinheadmz commented on issue "Coin Controll for Unconfirmed Outputs":
(https://github.com/bitcoin/bitcoin/issues/27190#issuecomment-1452166921)
I didn't have any trouble spending 0-confirmation inputs using Bitcoin-Qt (on regtest) by enabling the coin selection options





💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#issuecomment-1452168761)
> After more thorough testing I found the following issue, I think this pre-opened windows should be hidden or closed.

Good catch! I'll give it a go... cheers.

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1452172358)
force-push to b4ce309ef5f2bed5ed1f861305317a0dca3dc5f4:

- addressed all nits and review comments by @LarryRuane
- added 1ms delay in blockmanager test for all non-windows systems (cc: @mzumsande )

👋 pinheadmz's pull request is ready for review: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101)

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1452190011)
> consider the suggestion for the unit test commit comment here: [#27039 (review)](https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1320666918) (I think you may have just missed it), but it's not important.

ah doh! I added a sentence to the PR description but if I rebase again I'll add to the actual commit

💬 MarcoFalke commented on issue "Coin Controll for Unconfirmed Outputs":
(https://github.com/bitcoin/bitcoin/issues/27190#issuecomment-1452194482)
I think the question is about untrusted inputs, not unconfirmed change