💬 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
(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.
(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.
(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
(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
(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.
(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
(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>
...
(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
...
(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.
(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
(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`.
(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.
(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
(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
(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.
(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?
(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
...
(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
...
(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.
(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.