💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1819360700)
Thanks! rebased.

💬 mzumsande commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1399433051)
> One small drawback of the current way of inspecting `extra_args` for the v2transport option is that it doesn't work if the argument is specified in a slightly different format with same meaning.

Good point - I agree that it makes sense to leave this for a possible follow-up. I'm not sur if we want to port all of the argsmanager logic into python, but having something a bit more general so that it could be reused by other args than just `-v2transport` in the future would be nice.

💬 darosior commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1819363878)
Concept ACK. Planning on reviewing soon.

💬 martinus commented on pull request "coins: make sure PoolAllocator uses the correct alignment":
(https://github.com/bitcoin/bitcoin/pull/28913#issuecomment-1819368254)
I've updated the test in the PR, this time I think it should have caught the issue. It should be reproducible by adding an `alignof(void*)` back as the `PoolAllocator`'s last template argument

👍 hebasto approved a pull request: "depends: bump libmultiprocess to fix capnproto deprecation warnings"
(https://github.com/bitcoin/bitcoin/pull/28907#pullrequestreview-1740227113)
ACK 21bfee0720ba72935d4f9fc4c2a2887ae5b54092, I have reviewed the code and it looks OK. I've also skimmed through the related changes in the https://github.com/chaincodelabs/libmultiprocess repository.

💬 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