💬 TheCharlatan commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2338228584)
Guix builds (aarch64):
```
b3bcd9f7508b35b3f4a187e9f1e4c87648bf3897575c0c4db8664f1fe08c2cf4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/SHA256SUMS.part
356c7b9493887fc839e6b7951a3396085bfea33efce91070f972a69f60aac1f4 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-be4f78275fa6-aarch64-linux-gnu-debug.tar.gz
e2887613ca5d1f929294487f86bd36ab6af0739d12625c6c661a35781c4f1523 guix-build-be4f78275fa6/output/aarch64-linux-gnu/bitcoin-be4f78275fa6-aarch64-linux-gnu.tar.gz
92514b096
...

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2338236543)
Thanks for the review fjahr!, it seems that your scripted-diff commit https://github.com/fjahr/bitcoin/commit/6cc25ffb1411285d8191bc932eb37b96348f022c makes CI unhappy. I haven't checked the reason but we could leave it for a follow-up to move forward with the fix for v28. Happy to check it.

✅ UdjinM6 closed a pull request: "fix: avoid calling `GetCoin` and `SignTransaction()` inside of `assert(...)` in tests"
(https://github.com/bitcoin/bitcoin/pull/29572)

💬 maflcko commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2338250464)
The xor patch didn't change buffering. Yes, in theory there is a 4096 byte max buffer, however, this is never hit in practice. Undo data consists of small integers (4 bytes or 1 byte) and short strings (like output scripts).

I expect that if you print the number of writes before and after, they are exactly the same. If you also print the size of the writes they should show a histogram where 1-byte writes are the most common (probably all varint), followed by various writes all shorter than 33
...

👍 TheCharlatan approved a pull request: "security-check: test for `_FORTIFY_SOURCE` usage in release binaries"
(https://github.com/bitcoin/bitcoin/pull/27038#pullrequestreview-2290087577)
ACK be4f78275fa6608b11377dd5a29a809597d3fe8d

💬 TheCharlatan commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#discussion_r1750369033)
Bit unfortunate that this list has to be maintained, but I can't think of a better way either.

👍 fanquake approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845#pullrequestreview-2290092420)
ACK ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750230284)
In commit "refactor: Make code more tolerant of constexpr std::arrays" (c4c102de7336e6c0cc2e5187feedf437ebadb565)

I'm not sure I see benefit of changing this from `unsigned char` to `uint8_t` now when there's a good chance this is going to be switched again to `std::byte` in the future. It could also be good eliminate the C cast on this line if it is changing anyway.

So not important, but would consider changing `uint8_t(size)` to `static_cast<value_type(size)>` here and changing `uint8_t`
...

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750247803)
In commit "Generalize `std::vector` push to `std::span` to accept `std::array`" (402101e20bc2086e9237c2e8a51471ac97069e01)

Title of this commit is misleading it does not mention CScript. It sounds like it is referring to a vector push method not a script push method. Would suggest something more like "Allow CScript operator<< to accept spans, not just vectors"

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750251238)
In commit "Add std::byte span overload to CScript" (e6c02b9f2544032481743b4f8d3148622ccc73dd)

I don't think these changes which don't have to do with CScript belong in a commit that is supposed to be adding a new operator to CScript.

Would suggest moving definition of `BytesToUInt8s` and `UInt8sToBytes` into a new commit before any of the cscript changes so this can be more understandable.

It would also be good if commit gave a hint about what purpose of these util test changes are. The
...

🤔 ryanofsky reviewed a pull request: "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2289823867)
Code review e6c02b9f2544032481743b4f8d3148622ccc73dd

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750292662)
In commit "Add std::byte span overload to CScript" (e6c02b9f2544032481743b4f8d3148622ccc73dd)

I think this could use some explanation because it seems unexpected and potentially unsafe to bit cast std::span objects and make assumptions about internal layout of std::span. I can see why bit casting std::array is ok because of restrictions it has, but unclear why bit-casting std::span objects is ok when std::span objects are just wrappers around pointers and bit-casting pointers is not ok. At le
...

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750204329)
In commit "refactor: Make code more tolerant of constexpr std::arrays" (c4c102de7336e6c0cc2e5187feedf437ebadb565)

Sorry I didn't read the previous threads which might explain this, but this is a very vague comment which should be improved. It's is not clear looking at the new code what "temporary expression" or "explicit type" the comment is referring to, what "calms down GCC" could mean or what "some circumstances" could be. Would suggest writing a more understandable comment like `// Passin
...

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750308907)
In commit "Add std::byte span overload to CScript" (e6c02b9f2544032481743b4f8d3148622ccc73dd)

If code is moving away from `char` and `uint8_t` and towards `std::byte`, it would seem better for the `uint8_t` overload to call the `std::byte` overload (with `UInt8sToBytes`) instead of vice versa so the uint8_t overload could be dropped in the future without having to change this code again.

💬 furszy commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750378079)
> > I think we should allow assumeUTXO snapshot customization on regtest. It would make testing much simpler and self-contained.
>
> How would it make it simpler? It's another thing to keep track of in the test code and the implementation code.

The custom snapshot feature shouldn't take more than few lines at `init.cpp`. And for the test side, it would be an extra argument during init. I believe we will sooner or later need different snapshot for further wallet tests, maybe more than one i
...

🚀 fanquake merged a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845)

💬 fanquake commented on pull request "build: Use correct variable name":
(https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2338328011)
ACK - post rebase.

👋 hebasto's pull request is ready for review: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791)

💬 hebasto commented on pull request "build: Use correct variable name":
(https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2338334624)
Rebased on top of the merged https://github.com/bitcoin/bitcoin/pull/30845.

💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#issuecomment-2338335978)
> ACK [d4c7c40](https://github.com/bitcoin/bitcoin/commit/d4c7c4009da14c84576d43ab4a1241967cfa5ffc)
>
> This code gives me a headache 🤯 (the old one, the new one - less)

This is literally the reason why I decided to fix this 🫠

https://www.notion.so/srgi/PR-29415-110fd8ede41446fba41e81859bbdf985?pvs=4#521d59ebadd2475bb5630fbad147cbe4