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

🤔 hebasto reviewed a pull request: "ci: Rename tasks (previous releases, macOS cross)"
(https://github.com/bitcoin/bitcoin/pull/29218#pullrequestreview-1819952922)
Post-merge ACK fa0c594b33970e12d97e6879ab4ca57045453492.

@maflcko

Thank you for addressing https://github.com/bitcoin/bitcoin/pull/29203#discussion_r1446339387.

💬 hebasto commented on pull request "build: Drop `ALLOW_HOST_PACKAGES` support in depends":
(https://github.com/bitcoin/bitcoin/pull/29203#discussion_r1451423102)
Thanks for https://github.com/bitcoin/bitcoin/pull/29218 :)

💬 hebasto commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1890400163)
> It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.

Squashed.

👍 hebasto approved a pull request: "depends: Allow PATH with spaces in directory names."
(https://github.com/bitcoin/bitcoin/pull/29237#pullrequestreview-1819967328)
ACK 4756114e505cff8848fb6344ef9a48d8822066c1, successfully built depends on Ubuntu 22.04.