💬 maflcko commented on pull request "[POC] ci: Skip compilation when running static code analysis":
(https://github.com/bitcoin/bitcoin/pull/32953#discussion_r2204607453)
It seems fine to wait a few months until the 26.04 tag is available and then just use that. Also, it seems fine to just require cmake 3.31 (or higher) if anyone wants to use the codegen target, but no strong opinion.
(https://github.com/bitcoin/bitcoin/pull/32953#discussion_r2204607453)
It seems fine to wait a few months until the 26.04 tag is available and then just use that. Also, it seems fine to just require cmake 3.31 (or higher) if anyone wants to use the codegen target, but no strong opinion.
💬 fanquake commented on issue "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]":
(https://github.com/bitcoin/bitcoin/issues/32965#issuecomment-3069047625)
https://github.com/bitcoin/bitcoin/actions/runs/16263971992/job/45915380111#step:13:114
(https://github.com/bitcoin/bitcoin/issues/32965#issuecomment-3069047625)
https://github.com/bitcoin/bitcoin/actions/runs/16263971992/job/45915380111#step:13:114
💬 Sjors commented on pull request "descriptor: don't underestimate the size of a Taproot spend (instead, overestimate it)":
(https://github.com/bitcoin/bitcoin/pull/32964#issuecomment-3069141228)
I don't think we should be overpaying by default, for two reasons:
1. it's expensive
2. it reveals the presence of script paths
An intermediate solution could be that we never sign script paths by default, i.e. make `keypath_only` the default after #32857. Only when the user opts in to script path spending do we apply this solution of using the worst case fee. Since at least that's better than the current "solution" of manually picking a higher fee rate.
(https://github.com/bitcoin/bitcoin/pull/32964#issuecomment-3069141228)
I don't think we should be overpaying by default, for two reasons:
1. it's expensive
2. it reveals the presence of script paths
An intermediate solution could be that we never sign script paths by default, i.e. make `keypath_only` the default after #32857. Only when the user opts in to script path spending do we apply this solution of using the worst case fee. Since at least that's better than the current "solution" of manually picking a higher fee rate.
💬 Sjors commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069171378)
> Over that period the USD price of BTC has risen by roughly 2-3 orders of magnitude, making the economic cost of relaying small transactions disproportionately high.
The question is whether DoS protection back then was actually sufficient, or if the increase in fiat terms was the luck we needed. I don't know the answer to that.
Another difference between now and back then is the presence of Lightning and other systems that are sensitive to mempool shenanigans in general. Again I don't if
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069171378)
> Over that period the USD price of BTC has risen by roughly 2-3 orders of magnitude, making the economic cost of relaying small transactions disproportionately high.
The question is whether DoS protection back then was actually sufficient, or if the increase in fiat terms was the luck we needed. I don't know the answer to that.
Another difference between now and back then is the presence of Lightning and other systems that are sensitive to mempool shenanigans in general. Again I don't if
...
💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2204657999)
In 89831dace9d21456eb4a019d86164304e22f458e _wallet/rpc: add create silent-payments wallet option_: while testing I found that it doesn't disable block filters.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2204657999)
In 89831dace9d21456eb4a019d86164304e22f458e _wallet/rpc: add create silent-payments wallet option_: while testing I found that it doesn't disable block filters.
🤔 fanquake requested changes to a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3016058319)
This makes the CI dependant on being run on x86_64; the tidy job will no-longer pass when run on aarch64.
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3016058319)
This makes the CI dependant on being run on x86_64; the tidy job will no-longer pass when run on aarch64.
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069275601)
may be good to fix the aarch64 ci. Otherwise:
re-ACK bfbf7de389649f848dffe23be6727df715648b01 🏖
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wL
...
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069275601)
may be good to fix the aarch64 ci. Otherwise:
re-ACK bfbf7de389649f848dffe23be6727df715648b01 🏖
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wL
...
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069279294)
> may be good to fix the aarch64 ci.
How would you fix it, given IWYU is giving different recommendations, based on the architecture?
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069279294)
> may be good to fix the aarch64 ci.
How would you fix it, given IWYU is giving different recommendations, based on the architecture?
💬 flack commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069313602)
small nit: The PR description should rather point to this commit: 9e93640be6c49fa1505ba5c5df8c89210da5a6e4
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3069313602)
small nit: The PR description should rather point to this commit: 9e93640be6c49fa1505ba5c5df8c89210da5a6e4
💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069317099)
ACK 44f3bae300dcafbe53f9b07e6cc22a112833e579
> Yikes. Concept ACK.
Yea. It's an interesting example of how bumping/changing a CMake minimum version, can opt you into, or out of, unexpected behaviour (I imagine this is why this stopped being the default).
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069317099)
ACK 44f3bae300dcafbe53f9b07e6cc22a112833e579
> Yikes. Concept ACK.
Yea. It's an interesting example of how bumping/changing a CMake minimum version, can opt you into, or out of, unexpected behaviour (I imagine this is why this stopped being the default).
✅ fanquake closed an issue: "cmake: searching across directories for config files"
(https://github.com/bitcoin/bitcoin/issues/32938)
(https://github.com/bitcoin/bitcoin/issues/32938)
🚀 fanquake merged a pull request: "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`"
(https://github.com/bitcoin/bitcoin/pull/32943)
(https://github.com/bitcoin/bitcoin/pull/32943)
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3069321057)
reACK 3afc73134bf652d01e25232701c2ef192b3fea0b
just a reminder that you don't need to rebase on master unless a silent merge conflict is detected or drahtbot yells at you about an issue merging
`git range-diff master 4ddc577953a7407ea44a3e3d8d9a4635929307ca 3afc73134bf652d01e25232701c2ef192b3fea0b`
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3069321057)
reACK 3afc73134bf652d01e25232701c2ef192b3fea0b
just a reminder that you don't need to rebase on master unless a silent merge conflict is detected or drahtbot yells at you about an issue merging
`git range-diff master 4ddc577953a7407ea44a3e3d8d9a4635929307ca 3afc73134bf652d01e25232701c2ef192b3fea0b`
💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069329852)
Backported to 29.x in #32863.
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069329852)
Backported to 29.x in #32863.
💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069344912)
> How would you fix it, given IWYU is giving different recommendations, based on the architecture?
Easiest way is to use `// IWYU pragma: keep` when IWYU incorrectly tells you to remove an include that should be kept and `// IWYU pragma: no_include` when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms. I dealt with this recently in https://github.com/bitcoin-core/libmultiprocess/comm
...
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069344912)
> How would you fix it, given IWYU is giving different recommendations, based on the architecture?
Easiest way is to use `// IWYU pragma: keep` when IWYU incorrectly tells you to remove an include that should be kept and `// IWYU pragma: no_include` when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms. I dealt with this recently in https://github.com/bitcoin-core/libmultiprocess/comm
...
📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32969)
Backports:
* #32943
(https://github.com/bitcoin/bitcoin/pull/32969)
Backports:
* #32943
💬 fanquake commented on pull request "depends: Force `CMAKE_EXPORT_NO_PACKAGE_REGISTRY=TRUE`":
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069385247)
Backported to 28.x in #32969.
(https://github.com/bitcoin/bitcoin/pull/32943#issuecomment-3069385247)
Backported to 28.x in #32969.
📝 maflcko opened a pull request: "ci: Enable more shellcheck"
(https://github.com/bitcoin/bitcoin/pull/32970)
shellcheck is often the main "reviewer" of CI code written in Bash, so it seems odd to disable it by putting commands into `bash -c "cmd..."`.
Fix that by removing `bash -c`, where it isn't intended.
(https://github.com/bitcoin/bitcoin/pull/32970)
shellcheck is often the main "reviewer" of CI code written in Bash, so it seems odd to disable it by putting commands into `bash -c "cmd..."`.
Fix that by removing `bash -c`, where it isn't intended.
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069459525)
> `// IWYU pragma: no_include` when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms.
I wouldn't recommend to do this, because this can lead to missing includes, as also mentioned earlier in https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125793574. The correct fix is to just add the includes iwyu is asking for.
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3069459525)
> `// IWYU pragma: no_include` when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms.
I wouldn't recommend to do this, because this can lead to missing includes, as also mentioned earlier in https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2125793574. The correct fix is to just add the includes iwyu is asking for.
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3069532178)
@User087 There are a few differences from the feature you linked. From the same guide, [the very last section](https://github.com/monero-project/monero/blob/master/docs/ANONYMITY_NETWORKS.md#i2ptor-stream-used-twice):
> I2P/Tor Stream Used Twice
`monerod` currently selects two outgoing connections every 5 minutes for transmitting transactions over I2P/Tor.
The proposed feature here does not use any existing connections for transmitting transactions. It creates a new connection to a random p
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3069532178)
@User087 There are a few differences from the feature you linked. From the same guide, [the very last section](https://github.com/monero-project/monero/blob/master/docs/ANONYMITY_NETWORKS.md#i2ptor-stream-used-twice):
> I2P/Tor Stream Used Twice
`monerod` currently selects two outgoing connections every 5 minutes for transmitting transactions over I2P/Tor.
The proposed feature here does not use any existing connections for transmitting transactions. It creates a new connection to a random p
...