🤔 jonatack reviewed a pull request: "doc, test: test and explain service flag handling"
(https://github.com/bitcoin/bitcoin/pull/29213#pullrequestreview-1819009947)
ACK d536e5a6325d1885224f36debdcc5245b94efe8a

💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450936649)
It seems to me this might be helpful and the message names are unlikely to change.

```diff
// Overwrites potentially existing services. In contrast to this,
- // unvalidated services received via gossip relay are only
- // ever added but cannot replace existing ones.
+ // unvalidated services received via gossip relay in ADDR/ADDRV2
+ // messages are only ever added but do not replace existing ones.
```

💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450905336)
Perhaps add a "see `AddSingle()`" mention somewhere in here, as the logic referred to here is in that method called from this one (`Add/Add_`).

💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450951378)
```suggestion
// Adding service flags even works when the addr is in Tried; see `AddSingle()`
```

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890013810)
Gawd, SFFO is such pain in the neck.

💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450962291)
> Again, no opinion on this. I think either solution is fine. PRs welcome, I guess?

Thanks, I started playing around with a change that should make it easier to set the category without being verbose, and also start relying less on a global logging instance, which should be useful for kernel applications and tests: 262923c5b37f62a925a2c5611318855009bfb241. Will see how this looks and probably open a PR.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890125580)
Fixed bug discovered with the optimality fuzz test: [opt: Skip over barren combinations of tiny UTXOs](https://github.com/bitcoin/bitcoin/pull/27877/commits/80bcee5c09fb68366645d6be62fdeab4bb3ec6f3), incorrectly was set to cut, but this could be premature in case that the last selected UTXO was heavier than later UTXOs in the pool.

📝 achow101 opened a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243)
AttachChain will create the chain notifications handler which contains a reference to the wallet's shared_ptr. If AttachChain fails, the wallet needs to be unloaded, and this is expected to happen with its custom deleter ReleaseWallet. However, if the chain notifications handler is still set, then the shared_ptr is still referenced by something, so the wallet is never actually released.

💬 achow101 commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1890229823)
This actually revealed quite a subtle bug in the way that we handle cleaning up after an `AttachChain` failure. I've opened a fix in #29243

💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1890230397)
I believe I've found the root cause of the test failure, fix in #29243

💬 jamesob commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890246257)
Huh, any clue as to why this only becomes an issue on Windows?

💬 murchandamus commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890247334)
Any idea why this didn’t fail in the CI on https://github.com/bitcoin/bitcoin/pull/28838 before getting merged?

💬 achow101 commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890247599)
> Huh, any clue as to why this only becomes an issue on Windows?

Presumably only Windows is checking whether other things have the file open before attempting to remove them. Since the other shared_ptr reference was still hanging around in memory, bitcoind still had the database files open when `RestoreWallet` tries to delete the files for its cleanup on failure.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890248185)
CI failure is unrelated, see https://github.com/bitcoin/bitcoin/pull/29243

💬 achow101 commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890248958)
> Any idea why this didn’t fail in the CI on #28838 before getting merged?

Apparently we weren't running on functional tests in the Windows CI until recently: #29059

💬 jamesob commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890285738)
ACK https://github.com/bitcoin/bitcoin/pull/29243/commits/ea2551e55d260854a5cca8aa95034970d4adca1c

CI is happy.

✅ jamesob closed a pull request: "test: unbreak: exclude windows from wallet_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/29238)

💬 jamesob commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1890285982)
Closing in lieu of https://github.com/bitcoin/bitcoin/pull/29243.

💬 maflcko commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890391142)
Is this for backport?

🤔 hebasto reviewed a pull request: "build: Bump clang minimum supported version to 14"
(https://github.com/bitcoin/bitcoin/pull/29208#pullrequestreview-1819951236)
Post-merge ACK aaaace2fd1299939c755c281b787df0bbf1747a0.