💬 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.
📝 glozow opened a pull request: "[29.x] 33106 backport and final changes for rc2"
(https://github.com/bitcoin/bitcoin/pull/33226)
Backports #33106 and includes final changes for 29.1rc2. Built on top of #33225. I'll rebase shortly after that's in.
I did not include #32750 because it causes #33177 and I don't foresee any problems; it was just a nice to have.
For reviewers: the backport is unclean but fairly straightforward. I just had to adapt a test that is no longer in master (#32973) and include `-datacarriersize` in order to pad transaction size (#32406).
(https://github.com/bitcoin/bitcoin/pull/33226)
Backports #33106 and includes final changes for 29.1rc2. Built on top of #33225. I'll rebase shortly after that's in.
I did not include #32750 because it causes #33177 and I don't foresee any problems; it was just a nice to have.
For reviewers: the backport is unclean but fairly straightforward. I just had to adapt a test that is no longer in master (#32973) and include `-datacarriersize` in order to pad transaction size (#32406).
🚀 glozow merged a pull request: "[29.x] Backport logging ratelimiting"
(https://github.com/bitcoin/bitcoin/pull/33225)
(https://github.com/bitcoin/bitcoin/pull/33225)
🤔 enirox001 reviewed a pull request: "wallet: Remove isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3136742267)
re-ACK be776a1
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3136742267)
re-ACK be776a1