💬 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).

💬 achow101 commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2338471211)
Is the cause for the speedup a difference in the actual hardware are simply due to dedicated resources?

I currently have some older server hardware that I am planning to dedicate to fuzzing. I could instead also set them up for our CI. These servers will live in a data center and should have 100% uptime.

🤔 mzumsande reviewed a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2290116946)
Concept ACK

I think the description of the first commit could be improved:
> AssumeUTXO nodes prioritize tip synchronization first, meaning they advertise
knowledge of all blocks posterior to the snapshot base block hash.

This is confusing to me - nodes advertise `NODE_NETWORK` (all historical blocks prior to tip) or `NODE_NETWORK_LIMITED` (last 288 from tip). They also communicate their current tip with peers, but it's not part of the advertisement. So this includes (amongst others
...

💬 mzumsande commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750386961)
whitespace nit: `& ~services` makes more sense to me, and clang-format agrees.