🤔 janb84 reviewed a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3136209292)
ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f (after base #31802)
This PR adds some functional tests for the IPC interface, setups the CI to use libmultiprocess and changes build documentation to match the changes in this PR.
Current state of the PR works on nix on macOS, builds & test run without trouble.
- build ✅
- tested ✅
- code review✅
small NIT if retouched, please see the remarks of DrahtBot's LLM linter.
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3136209292)
ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f (after base #31802)
This PR adds some functional tests for the IPC interface, setups the CI to use libmultiprocess and changes build documentation to match the changes in this PR.
Current state of the PR works on nix on macOS, builds & test run without trouble.
- build ✅
- tested ✅
- code review✅
small NIT if retouched, please see the remarks of DrahtBot's LLM linter.
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206024287)
> If this PR is merged as-is now, and later -ipcbind support is added to bitcoind too, then we end up in a situation where we're literally shipping two feature-equivalent binaries (bitcoin-node and bitcoind),
I don't see how that would make sense. We should simply drop the libexec/bitcoin-node and libexec/bitcoin-gui binaries if they do not have distinguishing features at that point.
I feel like I keep pointing out difference between the `bin/` and `libexec/` directories, and you and Cory
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206024287)
> If this PR is merged as-is now, and later -ipcbind support is added to bitcoind too, then we end up in a situation where we're literally shipping two feature-equivalent binaries (bitcoin-node and bitcoind),
I don't see how that would make sense. We should simply drop the libexec/bitcoin-node and libexec/bitcoin-gui binaries if they do not have distinguishing features at that point.
I feel like I keep pointing out difference between the `bin/` and `libexec/` directories, and you and Cory
...
💬 Crypt-iQ commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2287969247)
I think it could make sense to adjust for 29.x if users are running the functional tests, look at the functional test logs, and are confused why some tests have logs rate limited.
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2287969247)
I think it could make sense to adjust for 29.x if users are running the functional tests, look at the functional test logs, and are confused why some tests have logs rate limited.
💬 sipa commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206057126)
My concern is not the fact that there are added binaries, but that we force people to use them if they want the mining IPC but don't care about multiprocess - resulting in them using an approach that isn't tested/looked at/used by anyone else. I think the mental load is a factor too (though Cory seems to care more about this), but it is about having to explain to people that they now (in some cases) need `bitcoin -m node` rather than `bitcoind`. The fact that there are more binaries isn't nearly
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206057126)
My concern is not the fact that there are added binaries, but that we force people to use them if they want the mining IPC but don't care about multiprocess - resulting in them using an approach that isn't tested/looked at/used by anyone else. I think the mental load is a factor too (though Cory seems to care more about this), but it is about having to explain to people that they now (in some cases) need `bitcoin -m node` rather than `bitcoind`. The fact that there are more binaries isn't nearly
...
🤔 janb84 reviewed a pull request: "doc: unify `datacarriersize` warning with release notes"
(https://github.com/bitcoin/bitcoin/pull/33224#pullrequestreview-3136345319)
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Pr changes warning of `datacarriersize` to a friendlier one. The friendlier text aligns also with the release notes.
Given that deprecation not always results in removal (in this project), I find this warning message a better representation of the reality.
(https://github.com/bitcoin/bitcoin/pull/33224#pullrequestreview-3136345319)
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Pr changes warning of `datacarriersize` to a friendlier one. The friendlier text aligns also with the release notes.
Given that deprecation not always results in removal (in this project), I find this warning message a better representation of the reality.
💬 cedwies commented on pull request "Avoid file overwriting in fallback `AllocateFileRange` implementation":
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2287997515)
Good point, in strict ISO C that's UB. My understanding is that in our case this code only runs on POSIX systems (not Win/macOS), where `fseek(..., SEEK_END)` is well-defined for regular files. Or am I missing an issue here?
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2287997515)
Good point, in strict ISO C that's UB. My understanding is that in our case this code only runs on POSIX systems (not Win/macOS), where `fseek(..., SEEK_END)` is well-defined for regular files. Or am I missing an issue here?
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206116947)
> but it is about having to explain to people that they now (in some cases) need bitcoin -m node rather than bitcoind
Oh yes, I completely agree this is not ideal. It should be possible to just specify `ipcbind` on the command line or configuration file and the `bitcoin` command should pick it up and call the right binary without the need for `-m`. A suggested improvement listed in #31375 is:
>- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206116947)
> but it is about having to explain to people that they now (in some cases) need bitcoin -m node rather than bitcoind
Oh yes, I completely agree this is not ideal. It should be possible to just specify `ipcbind` on the command line or configuration file and the `bitcoin` command should pick it up and call the right binary without the need for `-m`. A suggested improvement listed in #31375 is:
>- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is
...
🤔 Zero-1729 reviewed a pull request: "doc: unify `datacarriersize` warning with release notes"
(https://github.com/bitcoin/bitcoin/pull/33224#pullrequestreview-3136389147)
LGTM
crACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Good catch; the new message tone is more aligned and communicates the intention better.
(https://github.com/bitcoin/bitcoin/pull/33224#pullrequestreview-3136389147)
LGTM
crACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442
Good catch; the new message tone is more aligned and communicates the intention better.
💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3206135519)
Diff to reproduce on Linux:
```diff
diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
index be7f1ee5a2..ada3280dbc 100644
--- a/src/util/fs_helpers.cpp
+++ b/src/util/fs_helpers.cpp
@@ -200,7 +200,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
}
ftruncate(fileno(file), static_cast<off_t>(offset) + length);
#else
-#if defined(HAVE_POSIX_FALLOCATE)
+#if 0
// Version using posix_fallocate
off_t nEndPos = (off_t)offset + length;
...
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3206135519)
Diff to reproduce on Linux:
```diff
diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
index be7f1ee5a2..ada3280dbc 100644
--- a/src/util/fs_helpers.cpp
+++ b/src/util/fs_helpers.cpp
@@ -200,7 +200,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
}
ftruncate(fileno(file), static_cast<off_t>(offset) + length);
#else
-#if defined(HAVE_POSIX_FALLOCATE)
+#if 0
// Version using posix_fallocate
off_t nEndPos = (off_t)offset + length;
...
👍 dergoegge approved a pull request: "[29.x] Backport logging ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/33225#pullrequestreview-3136407332)
utACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
Cherry-picked the commits myself locally and got the same result (except for the release note which was moved to `doc/release-notes.md`).
(https://github.com/bitcoin/bitcoin/pull/33225#pullrequestreview-3136407332)
utACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
Cherry-picked the commits myself locally and got the same result (except for the release note which was moved to `doc/release-notes.md`).
💬 cedwies commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3206161232)
ACK 2885bd0
The PR adjusts the -datacarrier/-datacarriersize deprecation warning to be less absolute and better match the release notes.
I think the new wording still communicates deprecation, but without overstating certainty about removal.
Code change is minimal and the functional test was updated accordingly.
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3206161232)
ACK 2885bd0
The PR adjusts the -datacarrier/-datacarriersize deprecation warning to be less absolute and better match the release notes.
I think the new wording still communicates deprecation, but without overstating certainty about removal.
Code change is minimal and the functional test was updated accordingly.
💬 fanquake commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206177685)
Guix Build (aarch64):
```bash
ccc8209475974ddef8da13c2b4d09c3798ed26f1f8ca64bfad5cbaca858af1c0 guix-build-0022e25333a8/output/aarch64-linux-gnu/SHA256SUMS.part
4f1cc05dcf87206cd2e6dd601098c0df5518bbc347b9b42a6dbadbd45a24dbde guix-build-0022e25333a8/output/aarch64-linux-gnu/bitcoin-0022e25333a8-aarch64-linux-gnu-debug.tar.gz
053fba6c3981b92e9c7af91149d49cae6a1f10d1e8c431e90cdde47eb1d0bab8 guix-build-0022e25333a8/output/aarch64-linux-gnu/bitcoin-0022e25333a8-aarch64-linux-gnu.tar.gz
8dfde4
...
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206177685)
Guix Build (aarch64):
```bash
ccc8209475974ddef8da13c2b4d09c3798ed26f1f8ca64bfad5cbaca858af1c0 guix-build-0022e25333a8/output/aarch64-linux-gnu/SHA256SUMS.part
4f1cc05dcf87206cd2e6dd601098c0df5518bbc347b9b42a6dbadbd45a24dbde guix-build-0022e25333a8/output/aarch64-linux-gnu/bitcoin-0022e25333a8-aarch64-linux-gnu-debug.tar.gz
053fba6c3981b92e9c7af91149d49cae6a1f10d1e8c431e90cdde47eb1d0bab8 guix-build-0022e25333a8/output/aarch64-linux-gnu/bitcoin-0022e25333a8-aarch64-linux-gnu.tar.gz
8dfde4
...
💬 josibake commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3206209759)
Thanks for taking the suggestion of splitting this up! I think it would be nice to add a blurb to the description that this PR depends on https://github.com/bitcoin/bitcoin/pull/33216 and is in draft pending review on the parent PR. You mention this in the comments, but having it in the description ensures reviewers dont miss it / get confused on the status of the PR.
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3206209759)
Thanks for taking the suggestion of splitting this up! I think it would be nice to add a blurb to the description that this PR depends on https://github.com/bitcoin/bitcoin/pull/33216 and is in draft pending review on the parent PR. You mention this in the comments, but having it in the description ensures reviewers dont miss it / get confused on the status of the PR.
👍 instagibbs approved a pull request: "rpc: followups for 33106"
(https://github.com/bitcoin/bitcoin/pull/33189#pullrequestreview-3136380951)
ACK daa40a3ff97346face9dcc64564010a66c91ccb2
(https://github.com/bitcoin/bitcoin/pull/33189#pullrequestreview-3136380951)
ACK daa40a3ff97346face9dcc64564010a66c91ccb2
💬 instagibbs commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288015854)
personal opinion only but printing 8 places always to easily eyeball the rate is better, so -0 on this suggestion
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288015854)
personal opinion only but printing 8 places always to easily eyeball the rate is better, so -0 on this suggestion
💬 instagibbs commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288034462)
small note with explanation on how miners can use it would be nice since some miners are opting out of mining such low feerates for now
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2288034462)
small note with explanation on how miners can use it would be nice since some miners are opting out of mining such low feerates for now
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206253497)
> My concern is not the fact that there are added binaries, but that we force people to use a different one (bitcoin-node, even if it's not visible) if they want the mining IPC but don't care about multiprocess - resulting in them using an approach that isn't tested/looked at/used by anyone else.
To be clear, this does not sound like a concern currently, but would be a concern if future PR's do something we **both** agree would be bad: automatically opt users into the process separation featu
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206253497)
> My concern is not the fact that there are added binaries, but that we force people to use a different one (bitcoin-node, even if it's not visible) if they want the mining IPC but don't care about multiprocess - resulting in them using an approach that isn't tested/looked at/used by anyone else.
To be clear, this does not sound like a concern currently, but would be a concern if future PR's do something we **both** agree would be bad: automatically opt users into the process separation featu
...
💬 instagibbs commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206261504)
utACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
re-applied all commits and only difference is release notes change location
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206261504)
utACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
re-applied all commits and only difference is release notes change location
💬 stickies-v commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288145855)
+1, small change that might turn out helpful during debugging
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288145855)
+1, small change that might turn out helpful during debugging
👍 stickies-v approved a pull request: "[29.x] Backport logging ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/33225#pullrequestreview-3136573025)
ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f - all backports clean except the release notes one, as indicated.
Can't see any weird interactions with the 29.x branch.
(https://github.com/bitcoin/bitcoin/pull/33225#pullrequestreview-3136573025)
ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f - all backports clean except the release notes one, as indicated.
Can't see any weird interactions with the 29.x branch.