🚀 fanquake merged a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845)
(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.
(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)
(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.
(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
(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)
(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.
(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
...
(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
(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).
(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.
(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
...
(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.
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1750386961)
whitespace nit: `& ~services` makes more sense to me, and clang-format agrees.
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2338512812)
I haven't done the benchmarks here, so I can't say for sure, but the `AMD EPYC™ 9454P` all-core boost clock is lower than the base clock of the `AMD Ryzen™ 7 7700` that @willcl-ark did the benchmark on. So I guess it is a combination of faster clock + dedicated resources + more idle CPU waiting for work on the server?
If your older hardware has about 90 CPU threads, about 152 GB of RAM, and a 3 TB of SSD, and has comparable performance, it could also be used.
I think you can compare the pe
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2338512812)
I haven't done the benchmarks here, so I can't say for sure, but the `AMD EPYC™ 9454P` all-core boost clock is lower than the base clock of the `AMD Ryzen™ 7 7700` that @willcl-ark did the benchmark on. So I guess it is a combination of faster clock + dedicated resources + more idle CPU waiting for work on the server?
If your older hardware has about 90 CPU threads, about 152 GB of RAM, and a 3 TB of SSD, and has comparable performance, it could also be used.
I think you can compare the pe
...
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2338518268)
Asan is running on GHA infra, so this highlights that the issue may not be infra related, but related to the code, config or build files in this repo?
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2338518268)
Asan is running on GHA infra, so this highlights that the issue may not be infra related, but related to the code, config or build files in this repo?
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2338522744)
reACK 59028de422bbf8ccf53da27f1153e343952e585f
via `git range-diff master 100e1b9ecb29c76187112fda9fed1c01da192c99 59028de422bbf8ccf53da27f1153e343952e585f`
fwiw I ran the branch with the two dropped optimizations, and it lead to a slight improvement to most of the optimal benchmarks on my machine
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2338522744)
reACK 59028de422bbf8ccf53da27f1153e343952e585f
via `git range-diff master 100e1b9ecb29c76187112fda9fed1c01da192c99 59028de422bbf8ccf53da27f1153e343952e585f`
fwiw I ran the branch with the two dropped optimizations, and it lead to a slight improvement to most of the optimal benchmarks on my machine
💬 m3dwards commented on issue "Trying to run bitcoin qt on Windows and getting an AV":
(https://github.com/bitcoin/bitcoin/issues/30825#issuecomment-2338560259)
So far unable to recreate on Win 11. Installing the 23H2 update and will try again.
(https://github.com/bitcoin/bitcoin/issues/30825#issuecomment-2338560259)
So far unable to recreate on Win 11. Installing the 23H2 update and will try again.
🚀 achow101 merged a pull request: "fix: increase consistency of rpcauth parsing"
(https://github.com/bitcoin/bitcoin/pull/30401)
(https://github.com/bitcoin/bitcoin/pull/30401)
💬 achow101 commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2338581569)
ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2338581569)
ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef
🚀 achow101 merged a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684)
(https://github.com/bitcoin/bitcoin/pull/30684)