💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2188786279)
Rebased after #30200 landed! 🎉 Reorganized commits so it's now based on #30332.

💬 maflcko commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#discussion_r1652705500)
See "Lost baseline coverage " in https://corecheck.dev/bitcoin/bitcoin/pulls/30324. So this seems wrong

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2188840483)
I dropped b9fdd0dc75b5b4944dffc700b0391b38465f754a as suggested by @ryanofsky so the PR is focussed on Cirrus.

I updated 026d86427527a6c58425896066386713b9782275 to drop the `bitcoin-core/gui` workaround. Updated the PR description to make clear that repo needs to have `NO_BRANCH=true` set _before_ merging.

https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652116935

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