💬 maflcko commented on issue "ci: failure in win64 unit tests `err:virtual:virtual_alloc_first_teb wine: failed to map the shared user data: c0000018\n0444:err:winediag:nodrv_CreateWindow Application tried to create a window, but no driver could be loaded.\n0444:err:winediag:nodrv_CreateWindow L"Make sure that your X server is running and that $DISPLAY is set correctly."`)":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2442017046)
Let's move discussion to #31071

✅ maflcko closed an issue: "win64-cross CI timeout: `wine: chdir to /tmp/wine-JFrcnM/server-30-e86edd : No such file or directory`; `01e0:err:module:relocate_ntdll ntdll could not be mapped at preferred address (0x320000), expect trouble`"
(https://github.com/bitcoin/bitcoin/issues/30969)

💬 maflcko commented on issue "win64-cross CI timeout: `wine: chdir to /tmp/wine-JFrcnM/server-30-e86edd : No such file or directory`; `01e0:err:module:relocate_ntdll ntdll could not be mapped at preferred address (0x320000), expect trouble`":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2442018028)
Let's move discussion to #31071

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2442024926)
This thread is getting long, but from what I tell, these are the current unaddressed review comments:

https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780881370
https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1795202925
https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791790452
https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791789613
https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1794934340
https://github.com/bitcoin/bitcoin/pull/30997
...

🚀 fanquake merged a pull request: "test: Remove dead code from interface_zmq test"
(https://github.com/bitcoin/bitcoin/pull/30942)

💬 rkrux commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1819433982)
Thanks for clarifying, I was wondering whether the below line would cause `NODE_NONE` to show up as `n` but that should not be the case.

💬 laanwj commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2442197074)
concept ACK, windows 10 was released almost 10 years ago now, i don't think it makes sense to support anything older

💬 fanquake commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1819471476)
Also need to updated WINNT here.

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819506779)
Was the assert failed because of the changeset state, or something that could be hit in master?

`AcceptSubPackage` is called for each submission attempt of any subpackage, so I'm not sure why `quit_early` would be problematic.

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819507957)
think this is right, you can resolve this

💬 hebasto commented on pull request "depends: Specify CMake generator explicitly":
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2442295413)
My Guix build:
```
aarch64
2b8363e54965bc413159a4dbc371a70e4c8a77976c196c57747917928009ed03 guix-build-e2ba8236715e/output/aarch64-linux-gnu/SHA256SUMS.part
4b0b3358ffe8c136759ff8594e5ea4502a7b9299f3c3b271c65bc3187b832960 guix-build-e2ba8236715e/output/aarch64-linux-gnu/bitcoin-e2ba8236715e-aarch64-linux-gnu-debug.tar.gz
303f29d256c2f31909b8a6ffdbcb2c6fd974a3e188bb6726de369c8746ad4b9f guix-build-e2ba8236715e/output/aarch64-linux-gnu/bitcoin-e2ba8236715e-aarch64-linux-gnu.tar.gz
df579740
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819536386)
pretty sure f497414ce76a4cf44fa669e3665746cc17710fc6 means you can't `Assume` anymore?

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819473572)
minor spelling: OutputGroups

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819508593)
Is this name going to set this portion of tests apart enough to be meaningful? It seems only some of the BnB tests that were present in coinselector_tests have been refactored

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819541160)
This segment of code appears to me to be intended to refactor the make_hard_case exhaustion tests, but with differing amounts (17,18 vs. 14,17) and construction (amount = iterative CENT + i vs. bitshifting?). Isn't there double the coins being created in the make_hard_case? I appreciate that this format is more readable, though it's unclear to me why one should exhaust based on the given coin sizes and quantity. Is this related to src/wallet/coinselection.cpp setting TOTAL_TRIES to 100,000 cons
...

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819476976)
Can we eliminate a call to GetFee via reordering assignment?
```
dcsp.m_change_fee = /*155 sats=*/dcsp.m_effective_feerate.GetFee(dcsp.change_output_size);
dcsp.min_viable_change = /*204 sats=*/dcsp.m_discard_feerate.GetFee(dcsp.change_spend_size);
dcsp.m_cost_of_change = /*204 + 155 sats=*/dcsp.min_viable_change + dcsp.m_change_fee;
```

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819544428)
Worth noting that src/bench/coin_selection.cpp makes a copy of the make_hard_case function (commenting as such), so removal here may lead to confusion unless that benchmark is also updated.

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2442393053)
Force-pushed to address merge conflict from https://github.com/bitcoin/bitcoin/pull/31042, no other changes.

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819601522)
disregard this comment. I skipped your intent in the description

💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2442428828)
Concept ACK and code review ACK. This needs a more useful commit message though.

I wasn't familiar with this c++17 functionality, and at first glance it looked scary to me (as though we might get multiple `cs_main`s instantiated).
[
According to cppreference](https://en.cppreference.com/w/cpp/language/inline) though, this is a reasonable (if not more correct/modern) approach.

[This stackoverflow question/answers](https://stackoverflow.com/questions/56876624/inline-stdmutex-in-header-file
...