💬 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

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

Aha, yes I see those inputs are not listed in the GUI, I can look in to adding this as a feature

💬 ryanofsky commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123429117)
In commit "refactor: Use move semantics in `CCheckQueue::Add`" (bee19efe7d8f1d6c6e2be0eefa9745de2d42066d)

Previous code seems to be trying to boost performance by reusing the vector. Why is it changed here to not reuse the vector? Change seems orthogonal to the rest of the PR

💬 1440000bytes commented on issue "Coin Controll for Unconfirmed Outputs":
(https://github.com/bitcoin/bitcoin/issues/27190#issuecomment-1452222743)
> > I think the question is about untrusted inputs, not unconfirmed change
>
> Aha, yes I see those inputs are not listed in the GUI, I can look in to adding this as a feature

Its not considered safe and I doubt reviewers would ACK it