💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754251)
Fixed `NO_ARM=true`

Joined the paragraphs.

Explained what happens when you do neither.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754406)
Fixed.

I added an instruction at the top of this PR for where to set `NO_BRANCH=true`.

I can't find a Cirrus documentation page to point to for this, but you'll find it once you find the repo settings page.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754504)
Done

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652754589)
Done

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652761548)
My bad, I assumed that since nothing was printed it was omitted, but
```
if (m_print_modified_fee) {
throw std::runtime_error("m_print_modified_fee is not implemented");
}
```
fails - thanks for pointing it out.

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652776871)
I've fixed the other Sonarcould warning in a separate commit, if you think it's better that way, I'll merge it with the first one: https://github.com/bitcoin/bitcoin/pull/30324/commits/71753aca8535764f077dc737e92784d2e56f40ef

🤔 stickies-v reviewed a pull request: "rest: don't copy data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2138471543)
Code LGTM 9c9375cf3b581c31f047b87a2c05507dc7b31804 - just needs squashing.

nit: could use `std::span` to avoid merge conflict with #29119?

<details>
<summary>git diff on 785982458f</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index adc6040d08..b6c6db8b35 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -26,6 +26,7 @@
#include <deque>
#include <memory>
#include <optional>
+#include <span>
#include <string>
#include <unordered_map>

...

💬 stickies-v commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1652682123)
nit: we typically include our own headers first, and include in `httpserver.cpp` is missing

<details>
<summary>git diff</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index adc6040d08..6ba8754485 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -13,6 +13,7 @@
#include <netbase.h>
#include <node/interface_ui.h>
#include <rpc/protocol.h> // For HTTP status codes
+#include <span.h>
#include <sync.h>
#include <util/check.h>
#include <util/s
...

💬 fanquake commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652780400)
I don't think you need to add this comment.

💬 paplorinc commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652785346)
Removed, left it only in the commit message

💬 laanwj commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2188921013)
> nit: could use std::span to avoid merge conflict with https://github.com/bitcoin/bitcoin/pull/29119?

i think the problem with prematurely porting PRs to `std::span` is that utility functions (at least `ToBytes`) haven't been ported to `std::span`.

💬 stickies-v commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2188942111)
> i think the problem with prematurely porting PRs to `std::span` is that utility functions (at least `ToBytes`) haven't been ported to `std::span`.

I think it's fine to just use `std::to_bytes` (as done in my [diff](https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2138471543))? No other utilities needed here. Anyway, it's a nit - either is fine for me.

👍 TheCharlatan approved a pull request: "build: Bump clang minimum supported version to 16"
(https://github.com/bitcoin/bitcoin/pull/30263#pullrequestreview-2138675243)
ACK fa58c75880334cc58ccc8e5d491df8f0e587bf42

💬 TheCharlatan commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#issuecomment-2188967378)
Concept ACK

💬 laanwj commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2188974236)
Oh, hadn't seen that diff, sorry, somehow expected it to be more involved.

💬 paplorinc commented on pull request "refactor: Simplify SipHash and fix benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652834148)
@sipa, can you please confirm that the previous benchmark was indeed incorrect?

💬 stickies-v commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2189003762)
> ...it probably makes sense to just pick Sv2 to be that interface as it's designed exactly for that purpose

I agree we shouldn't reinvent the wheel and just implement the parts of sv2 that we need to improve our `getblocktemplate` functionality. I would just prefer to be much more strict in cutting out all the parts that Core doesn't require to support sv2 downstream, i.e. I don't think the "out-of-the-box" aspect should at at all be a priority when that can reasonably be addressed with exte
...

💬 maflcko commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652865972)
One reason to assign the result is to give the compiler a hint that the result is uses (and should not be discarded). Otherwise, the compiler may optimize away the calculation (and the whole benchmark).

Another reason is to modify the value that is being hashed. Otherwise, the compiler may detect that the same value is hashed in every loop iteration and will only calculate it once, effectively optimizing away the calculation (and the whole benchmark). (While I don't think it matters here, in
...

💬 paplorinc commented on pull request "refactor: Simplify SipHash and fix related benchmarks":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1652876883)
Yes, I understand the reason, that's why `doNotOptimizeAway` is needed in some cases.
But here it seems to produce the wrong result. I have validated it with a simple timed test, calculating a million different hashes with the two versions of the algorithm, which were giving different benchmarking speeds - and this new version resembles it more closely.
Also, we're not using this format in any other benchmarks I've seen.

👍 TheCharlatan approved a pull request: "guix: build with glibc 2.31"
(https://github.com/bitcoin/bitcoin/pull/29987#pullrequestreview-2138770843)
ACK b5fc6d46a3854c18f6e8dfc89540d24ef778caa2