💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611586817)
Yeah, tsan+bdb+aarch64 is impossible to run. You'll have to disable it.

💬 furszy commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1611601851)
Hmm interesting.
#27607 is getting quite close now. It's making some substantial changes to the index `Init` and `Start` code.
Will rebase furszy@8495c85 on top and push it so we can try that one too.

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1611602478)
> No need to "bless" a specific compiler, (current discussion includes switching from Clang to GCC as a workaround).

I think this is the same as point one, which is about the "Source and destination overlap in memcpy" false positive? If so, it could make sense to combine it into one point.

> Can use latest (hopefully less buggy) Valgrind, rather than whatever the distro happens to package.

Does it mean we drop support for distro-shipped valgrind? Might be good to clarify, and also might
...

💬 fjahr commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/27974#issuecomment-1611648809)
Looks like a dublicate of #27249 ?

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1245423693)
Thanks for looking into it. I like the idea of keeping them separate and avoiding including unneeded code, in general and for building with slower, older CPUs. So 👍 for your diff in https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244795380. Great work so far AFAICT. Will continue reviewing the remaining commits.

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1245427231)
> In both cases all tests would be recompiled

Yes, but future changes to `util/net` would not require compiling all the tests.

💬 fjahr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#discussion_r1245435268)
Hm, how would that happen? I didn't have any issues when testing this code with 2.2.1-alpha and 2.1.12-stable.

📝 sipa opened a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993)
Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
* The additionally authenticated data (AAD), padded to 16 bytes.
* The ciphertext, padded to 16 bytes.
* The length of the AAD and the length of the cipher
...

🤔 jonatack reviewed a pull request: "test: remove race in the user-agent reception check"
(https://github.com/bitcoin/bitcoin/pull/27986#pullrequestreview-1503489729)
Light tested ACK c1a169082ac1147ec2f3cc329f23bc0bfc28cd6b

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245457894)
Suggest while touching this, to make it clearer and more coherent with the documentation just above.

```diff
- # Consistency check that the Bitcoin Core has received our user agent string.
- # Find our connection in "getpeerinfo" by our address:port, it is unique.
+ # Consistency check that the node received our user agent string.
+ # Find our connection in getpeerinfo by our address:port, as it is unique.
```

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611751064)
`c1a169082a...28d26c4f37`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245457894

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245488264)
Done.

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245477180)
In commit "refactor: simplify pruning violation check" (2412ffdce69ce2b164e5303ae4b610ab0d00fd5c)

Looking at this code, it's not really clear that `block_to_test` is going to be non-null, and that the `CheckBlockDataAvailability` check is going to pass. If it were null, the pruning check would fail even if there were no pruned data.

So I think it would be good add an assert with an explanation of why `block_to_test` can't be null here, and I implemented this below.

While implementing th
...

👍 ryanofsky approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1503519460)
Code review ACK 2f830d1de8f0b57f46f1d7da3cb7b9ab4aa1e2ff. I left a long comment below about one of the intermediate commits after I got hung up on whether `block_to_test` could be null before it was passed to the CheckBlockData function. I think it is not too important since the code is deleted in the end, but I did suggest some improvements to CheckBlockData that I think would make it safer and easier to use.

👍 jamesob approved a pull request: "test: Fuzz on macOS"
(https://github.com/bitcoin/bitcoin/pull/27932#pullrequestreview-1503552896)
Github ACK https://github.com/bitcoin/bitcoin/pull/27932/commits/fae7c50d201726f605938c3511dd9119efeea5ec

Adds fuzzing to a new architecture, right? Seems like an easy win?

💬 jamesob commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1611769447)
Concept ACK. Thanks for filing the change @CaseyCarter, and welcome to the project!

👍 jamesob approved a pull request: "test: Fix intermittent issue in mining_getblocktemplate_longpoll.py"
(https://github.com/bitcoin/bitcoin/pull/27941#pullrequestreview-1503585748)
Github ACK https://github.com/bitcoin/bitcoin/pull/27941/commits/fa748c6f2ac2f9cac7ce42fd745ed3c3eae093b7

Seems like a commonsense fix? Can't test the RPC method if the Bitcoin Core HTTP handler hasn't booted.

👍 vasild approved a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1503590039)
ACK e7cf8657e1165ea5da3911a9e543837cd8938f97

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245517426)
Verified per https://docs.python.org/3/whatsnew/3.8.html that [asyncio.BaseTransport.get_extra_info()](https://docs.python.org/3/library/asyncio-protocol.html#asyncio.BaseTransport.get_extra_info) was added in Python 3.8, the minimum supported version per `doc/dependencies.md`.

I'm not sure if `import asyncio` should be added to this file.

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611783471)
ACK 28d26c4f37c433e35a4a05e144d05f0e464f8452

modulo question in https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245517426