💬 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

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750421415)
Yes, that's what I tried at first, but I don't yet see how that would be possible without changing `prevector`, as explained [here](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747621990)

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750424687)
Related to https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1748936794, will see what I can do to document it better. I guess the explanation should rather go to the commit message instead of the code, since the code itself isn't doing anything special.

🤔 ryanofsky reviewed a pull request: "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2290235809)
I think this PR has been a good exploration of ways to let CScript support std::span and std::byte but current approach is a more complicated than it needs to be. I think it's be worth considering a more minimal, focused change:

```diff
--- a/src/script/script.h
+++ b/src/script/script.h
@@ -463,7 +463,7 @@ public:
return *this;
}

- CScript& operator<<(const std::vector<unsigned char>& b) LIFETIMEBOUND
+ CScript& operator<<(std::span<const std::byte> b) LIFETIMEB
...

💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750460628)
> Yes, that's what I tried at first, but I don't yet see how that would be possible without changing `prevector`, as explained [here](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747621990)

Thanks that's helpful to know, and it's clear why changing prevector could fix this but not clear why it would be necessary to change prevector to fix that as opposed to just passing a start and end pointer of the type it is expecting

💬 fanquake commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2338464097)
Thought this might just be constrained to that job, but:
ASAN (currently running PR): [`Total Test time (real) = 1068.87 sec`](https://github.com/bitcoin/bitcoin/actions/runs/10775638367/job/29880507688?pr=30791#step:7:4709)
ASAN (master finished 23 minutes ago): [`Total Test time (real) = 498.22 sec`](https://github.com/bitcoin/bitcoin/actions/runs/10775288903/job/29879351290#step:7:4709).