💬 hodlinator commented on pull request "index: Don't commit state in BaseIndex::Rewind":
(https://github.com/bitcoin/bitcoin/pull/33212#discussion_r2287582650)
Right before the kill, an extra `getblockcount()`-call returns `start_height + 2`, same value for `getindexinfo().best_block_height`.
After the kill, we jump back exactly 2 blocks to `start_height`. I guess this is due to the clean shutdown in `restart_node()` on line 319 in the preceding test code above having fully committed that state to disk.
IIUC, could add a comment to that affect:
```suggestion
# Indexes reset to the point we last committed them to disk - during the clean
...
(https://github.com/bitcoin/bitcoin/pull/33212#discussion_r2287582650)
Right before the kill, an extra `getblockcount()`-call returns `start_height + 2`, same value for `getindexinfo().best_block_height`.
After the kill, we jump back exactly 2 blocks to `start_height`. I guess this is due to the clean shutdown in `restart_node()` on line 319 in the preceding test code above having fully committed that state to disk.
IIUC, could add a comment to that affect:
```suggestion
# Indexes reset to the point we last committed them to disk - during the clean
...
✅ Eunovo closed a pull request: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680)
(https://github.com/bitcoin/bitcoin/pull/29680)
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3205520649)
re: https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3200849464
> ### RFM?
>
> With all that, I feel PR has had enough to discussion and review to be ready for merge given its impact.
I think this is still ready for merge but obviously want to give this more time after the discussion yesterday. I would like to merge this today though, if this is reasonable.
To summarize the discussion yesterday I added the following sections to the [review summary](https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3205520649)
re: https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3200849464
> ### RFM?
>
> With all that, I feel PR has had enough to discussion and review to be ready for merge given its impact.
I think this is still ready for merge but obviously want to give this more time after the discussion yesterday. I would like to merge this today though, if this is reasonable.
To summarize the discussion yesterday I added the following sections to the [review summary](https://github.com/bitcoi
...
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3205591544)
@theuni wrote:
> Please take a step back and just think like a user. v29 shipped with bitcoind. They'll now see bitcoin, bitcoind, and bitcoin-node.
As discussed above `bitcoin-node` is not in `PATH` since #31679. This has the nice benefit of being able to drop the binary if we go for a different approach later of adding `-ipcbind` to `bitcoind` instead, which is discussed in #30983 (not sure if it's a good idea).
Once we instruct miners to use `bitcoin -m node -ipcbind=unix`, ideally t
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3205591544)
@theuni wrote:
> Please take a step back and just think like a user. v29 shipped with bitcoind. They'll now see bitcoin, bitcoind, and bitcoin-node.
As discussed above `bitcoin-node` is not in `PATH` since #31679. This has the nice benefit of being able to drop the binary if we go for a different approach later of adding `-ipcbind` to `bitcoind` instead, which is discussed in #30983 (not sure if it's a good idea).
Once we instruct miners to use `bitcoin -m node -ipcbind=unix`, ideally t
...
📝 fanquake opened a pull request: "[29.x] Backport logging ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/33225)
Backports:
* #32604
* #33011
* #33211
(https://github.com/bitcoin/bitcoin/pull/33225)
Backports:
* #32604
* #33011
* #33211
💬 fanquake commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2287838255)
Adjust?
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2287838255)
Adjust?
💬 fanquake commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3205733317)
Backported in #33225.
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3205733317)
Backported in #33225.
💬 fanquake commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3205733976)
Backported to 29.x in #33225.
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3205733976)
Backported to 29.x in #33225.
💬 fanquake commented on pull request "test: modify logging_filesize_rate_limit params":
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3205734127)
Backported to 29.x in #33225.
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3205734127)
Backported to 29.x in #33225.
💬 sipa commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3205872366)
> I think we both agree that change could be a followup for a future release and does not need to block the current PR, **but please let me know if I misunderstood and that is not the case, or if you changed your mind about this**.
I don't really see the point in that case.
My (mild) preference, if we are to have IPC in release binaries in 30.0, is to add `-ipcbind` support to `bitcoind`, and not include `bitcoin-node` in the release. This means everyone uses and tests the same binary, isn
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3205872366)
> I think we both agree that change could be a followup for a future release and does not need to block the current PR, **but please let me know if I misunderstood and that is not the case, or if you changed your mind about this**.
I don't really see the point in that case.
My (mild) preference, if we are to have IPC in release binaries in 30.0, is to add `-ipcbind` support to `bitcoind`, and not include `bitcoin-node` in the release. This means everyone uses and tests the same binary, isn
...
🤔 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`).