👍 pinheadmz approved a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-3097356180)
re-ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
Changes since last ACK:
`git range-diff ad01440~6..ad01440 37cd2c0764~9..37cd2c0764`
Address PR feedback adding a line to interface docs, add mining interface to the GUI for testing, and refactors the test commits.
I rebased this PR at this commit on to #32345 and pummeled the IPC-RPC interface for an hour with no crash, so I think it's important that those libmutliprocess changes get merged as well, and *probably* should be merged first?
...
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-3097356180)
re-ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
Changes since last ACK:
`git range-diff ad01440~6..ad01440 37cd2c0764~9..37cd2c0764`
Address PR feedback adding a line to interface docs, add mining interface to the GUI for testing, and refactors the test commits.
I rebased this PR at this commit on to #32345 and pummeled the IPC-RPC interface for an hour with no crash, so I think it's important that those libmutliprocess changes get merged as well, and *probably* should be merged first?
...
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3164396555)
guix build for verification
`
aarch64
daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1 guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60 guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
3275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511 guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
7b5988013467c397fd2f4c23
...
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3164396555)
guix build for verification
`
aarch64
daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1 guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60 guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
3275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511 guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
7b5988013467c397fd2f4c23
...
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3164401518)
guix build for verification
```
daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1 guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60 guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
3275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511 guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
7b5988013467c397fd2f4c23105fc90
...
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3164401518)
guix build for verification
```
daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1 guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60 guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
3275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511 guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
7b5988013467c397fd2f4c23105fc90
...
💬 m3dwards commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3164429197)
> Other stuff to check would be that this works at all and doesn't just re-run an ancient commit, like [#32989 (comment)](https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3133536724)
Just ran a test to specifically test for this by updating the default (master) branch and adding and removing the label and it correctly picked an updated merge commit based on the PR merged into the updated master.
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3164429197)
> Other stuff to check would be that this works at all and doesn't just re-run an ancient commit, like [#32989 (comment)](https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3133536724)
Just ran a test to specifically test for this by updating the default (master) branch and adding and removing the label and it correctly picked an updated merge commit based on the PR merged into the updated master.
💬 m3dwards commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260511885)
Can you elaborate a little more on what this would look like? Would this be different labels for different jobs? As in a label that runs the Previous Releases job and a different label that would run a different job?
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260511885)
Can you elaborate a little more on what this would look like? Would this be different labels for different jobs? As in a label that runs the Previous Releases job and a different label that would run a different job?
💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260543464)
I don't know. Not sure if we want to see pull requests spammed with "added and removed" label events (https://github.com/testing-cirrus-runners/bitcoin/pull/19#event-19014793971).
GitHub doesn't make this easy and I wonder if we may want to just spin our own CI to detect silent merge conflicts.
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260543464)
I don't know. Not sure if we want to see pull requests spammed with "added and removed" label events (https://github.com/testing-cirrus-runners/bitcoin/pull/19#event-19014793971).
GitHub doesn't make this easy and I wonder if we may want to just spin our own CI to detect silent merge conflicts.
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260557261)
how do evictions happen, if `export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:-500M}` is set to 500M in total for all tasks?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260557261)
how do evictions happen, if `export CCACHE_MAXSIZE=${CCACHE_MAXSIZE:-500M}` is set to 500M in total for all tasks?
💬 maflcko commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2260562862)
```suggestion
LogInfo("Failed to detect filesystem type for: %s", util::Join(error_paths, ", "));
```
nit: no need for newline in new code
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2260562862)
```suggestion
LogInfo("Failed to detect filesystem type for: %s", util::Join(error_paths, ", "));
```
nit: no need for newline in new code
💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260589218)
To give some more context: We are now trying to add 200+ lines of code to this repo which have to be maintained, but they cause label spam, and likely aren't sufficient to achieve the goal, because the ci-failed notification will go to the one adding the label and triggering the workflow?
If so, we'd have to create a comment (or other notification) anyway. So if additional glue code is needed, we might as well handle all code externally.
(https://github.com/bitcoin/bitcoin/pull/33145#discussion_r2260589218)
To give some more context: We are now trying to add 200+ lines of code to this repo which have to be maintained, but they cause label spam, and likely aren't sufficient to achieve the goal, because the ci-failed notification will go to the one adding the label and triggering the workflow?
If so, we'd have to create a comment (or other notification) anyway. So if additional glue code is needed, we might as well handle all code externally.
🤔 Crypt-iQ reviewed a pull request: "validation: detect witness stripping without re-running Script checks"
(https://github.com/bitcoin/bitcoin/pull/33105#pullrequestreview-3097524181)
ACK 370770fb361fdb745cfa7bb9abc7ee4a7833b83b modulo commit message change and test comment fix
(https://github.com/bitcoin/bitcoin/pull/33105#pullrequestreview-3097524181)
ACK 370770fb361fdb745cfa7bb9abc7ee4a7833b83b modulo commit message change and test comment fix
💬 Crypt-iQ commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260577684)
should be "P2SH-wrapped P2WPKH"
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2260577684)
should be "P2SH-wrapped P2WPKH"
💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260635134)
Good question; remote backends are not governed by client-side CCACHE_MAXSIZE settings, see https://ccache.dev/manual/4.11.3.html#_http_storage_backend for a bit more info.
Basically the onus is on the server to periodically run `ccache --trim-dir` to keep it under control (we have asked for more details on this process cc @m3dwards )
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260635134)
Good question; remote backends are not governed by client-side CCACHE_MAXSIZE settings, see https://ccache.dev/manual/4.11.3.html#_http_storage_backend for a bit more info.
Basically the onus is on the server to periodically run `ccache --trim-dir` to keep it under control (we have asked for more details on this process cc @m3dwards )
💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259731802)
In "[test] check miner doesn't select 0fee transactions"
2778e4691664c55d5699bca2b68f15796e8a9a75
Maybe we should test both ways.
1. When the `blockmintxfee_btc_kvb` is > 0 we verify that we never create 0 or less fee tx.
2. When a 0 or less fee tx is created it is because of `blockmintxfee_btc_kvb` being 0.
```suggestion
for txid in block_template_txids:
for txid in block_template_txids:
fee = node.getmempoolentry(txid)['fees']['base'
...
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259731802)
In "[test] check miner doesn't select 0fee transactions"
2778e4691664c55d5699bca2b68f15796e8a9a75
Maybe we should test both ways.
1. When the `blockmintxfee_btc_kvb` is > 0 we verify that we never create 0 or less fee tx.
2. When a 0 or less fee tx is created it is because of `blockmintxfee_btc_kvb` being 0.
```suggestion
for txid in block_template_txids:
for txid in block_template_txids:
fee = node.getmempoolentry(txid)['fees']['base'
...
💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259885809)
In "[test] check bypass of minrelay for various minrelaytxfee settings" d40de30fdf0470a4ec6712aef4d5e8ed40e5ea91
We dont need the cleanup here I think
```suggestion
def test_minrelay_in_package_combos(self)
```
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259885809)
In "[test] check bypass of minrelay for various minrelaytxfee settings" d40de30fdf0470a4ec6712aef4d5e8ed40e5ea91
We dont need the cleanup here I think
```suggestion
def test_minrelay_in_package_combos(self)
```
👍 ismaelsadeeq approved a pull request: "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee"
(https://github.com/bitcoin/bitcoin/pull/33106#pullrequestreview-3096282456)
Code review ACK a43e1b28b2899e1707e7867fd46efe9fcc08f241
Although it's worth mentioning that there is a scenario were the price could crash to the point where the default min relay allows for free relay or atleast close to free, but a counter argument to that is that even current default is susceptible to such scenario.
> minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represen
...
(https://github.com/bitcoin/bitcoin/pull/33106#pullrequestreview-3096282456)
Code review ACK a43e1b28b2899e1707e7867fd46efe9fcc08f241
Although it's worth mentioning that there is a scenario were the price could crash to the point where the default min relay allows for free relay or atleast close to free, but a counter argument to that is that even current default is susceptible to such scenario.
> minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represen
...
💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2260507858)
I think `blockmintxfee` should be deprecated and eventually be removed.
What should be in the block template should just depends on the mempool transactions
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2260507858)
I think `blockmintxfee` should be deprecated and eventually be removed.
What should be in the block template should just depends on the mempool transactions
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260658713)
Would it not be easier for Cirrus to just fix the reverse-age order to pick the latest instead? Or somehow work around this by embedding the epoch seconds in the ccache blob name?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260658713)
Would it not be easier for Cirrus to just fix the reverse-age order to pick the latest instead? Or somehow work around this by embedding the epoch seconds in the ccache blob name?
💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164662786)
> Also, the error string CMake is throwing, is also incorrect, as the compiler does support LTO.
CMake doesn't blame the compiler, rather itself: "CMake doesn't support IPO for current compiler".
For some reason, this occurs with the "Unix Makefiles" generator but not with the "Ninja" generator.
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164662786)
> Also, the error string CMake is throwing, is also incorrect, as the compiler does support LTO.
CMake doesn't blame the compiler, rather itself: "CMake doesn't support IPO for current compiler".
For some reason, this occurs with the "Unix Makefiles" generator but not with the "Ninja" generator.
💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164664412)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164664412)
cc @purpleKarrot
💬 maflcko commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3164698282)
This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it'll likely miss the milestone.
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3164698282)
This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it'll likely miss the milestone.