💬 hebasto commented on pull request "coins: make sure PoolAllocator uses the correct alignment":
(https://github.com/bitcoin/bitcoin/pull/28913#issuecomment-1819422908)
A failure in the "Win64 native, VS 2022" CI job is unrelated. See: https://github.com/bitcoin/bitcoin/pull/28905.

✅ dergoegge closed a pull request: "fuzz: Delete wallet_notifications"
(https://github.com/bitcoin/bitcoin/pull/28882)

💬 dergoegge commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1819438212)
🤷

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#issuecomment-1819452855)
CI failure is unrelated see https://github.com/bitcoin/bitcoin/pull/28905

💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-1819490214)
> I don't think a detailed line-by-line code review at this stage is a good use of your time.

That's not what I'm trying to do. I strategically / haphazerdly picked a dozen lines to ask questions about to get a better understanding of the whole thing. Based on IRC chat today, I'll wait now for the upcoming update.

💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399521672)
The math school memories are coming back :-)

👍 TheCharlatan approved a pull request: "ci: Avoid toolset ambiguity that MSVC can't handle"
(https://github.com/bitcoin/bitcoin/pull/28905#pullrequestreview-1740351974)
utACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a

🤔 theStack reviewed a pull request: "coins: make sure PoolAllocator uses the correct alignment"
(https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1740357119)
Concept ACK

Due to a lack of real hardware I've set up an emulated 32-bit ARM system using qemu, maybe someone finds this useful: https://gist.github.com/theStack/6eaeadd3fa1e521ed038b1ed93c101d6

(Of course, the obvious drawback is that it's very sloooow.)

👍 pinheadmz approved a pull request: "coins: make sure PoolAllocator uses the correct alignment"
(https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1740446175)
ACK d5b4c0b69e543de51bb37d602d488ee0949ba185

Synced up to height 472600 on ARM 32 no issues, < 1 GB RAM used.
Also confirmed the test fails without the patch:

`test/pool_tests.cpp(189): error: in "pool_tests/memusage_test": check resource.NumAllocatedChunks() >= min_num_allocated_chunks has failed [1 < 157]`

Great catch, great work!


<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK d5b4c0b69e543de51bb37d602d488ee0949ba185
-
...

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1819618919)
> is this the test log you observed?

Yes. Only in ~1% of runs with the previous version, but after changing the garbage size in `net.cpp` to the max I would get it consistently.
I think the reason is that usually bitcoind would terminate the connection (because the python side would wait for more bytes that wouldn't come), but when the garbage was close to the max, the python side would terminate it and throwing the exception.

> i've removed the P2PInterface::data_received() call which pe
...

💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1399622598)
done

💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1399622838)
nop! done.

💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1399622978)
done

💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1819639666)
force-pushed addressing:
https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1399346490
https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1399350026
https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1399351848

👍 hebasto approved a pull request: "coins: make sure PoolAllocator uses the correct alignment"
(https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1740613397)
re-ACK d5b4c0b69e543de51bb37d602d488ee0949ba185, the only change since my recent [review](https://github.com/bitcoin/bitcoin/pull/28913#pullrequestreview-1739334189) is an updated test.

I've tried to break the updated test with the diff as follows:
```diff
--- a/src/test/pool_tests.cpp
+++ b/src/test/pool_tests.cpp
@@ -163,7 +163,8 @@ BOOST_AUTO_TEST_CASE(memusage_test)
std::hash<int64_t>,
std::equal_to<int64_t>,

...

👍 pablomartin4btc approved a pull request: "ci: Avoid toolset ambiguity that MSVC can't handle"
(https://github.com/bitcoin/bitcoin/pull/28905#pullrequestreview-1740613746)
utACK 91d5bd8ac9a28725c735f8e6900bc85673bb190a

💬 kevkevinpal commented on pull request "lint: Report all lint errors instead of early exit":
(https://github.com/bitcoin/bitcoin/pull/28862#issuecomment-1819785994)
ACK [fa01f88](https://github.com/bitcoin/bitcoin/pull/28862/commits/fa01f884d3ac128f09bfa57ac2648a19a19d854e)

tested on my own fork and saw multiple lint errors
https://github.com/kevkevinpal/bitcoin/pull/9/checks?check_run_id=18865861937

💬 m3dwards commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1819804495)
@hebasto thank you for the great suggestion. I have added a test for it in the testing guide.

💬 furszy commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819817301)
The latest state is https://github.com/bitcoin/bitcoin/pull/28395#pullrequestreview-1651973742. I could tackle it if @murchandamus is busy with something else.

💬 brunoerg commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819833040)
So, if the problem is SFFO, can we fix fuzz by doing:
```diff
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index 4caf96b18d..1e040fb5e2 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -116,12 +116,14 @@ FUZZ_TARGET(coinselection)
}

// Run coinselection algorithms
- auto result_bnb = SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT);
-
...

💬 furszy commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819842590)
I don't think hiding issues under the carpet is a good path to follow.