👍 hebasto approved a pull request: "rpc: remove one more quote from non-string oneline description"
(https://github.com/bitcoin/bitcoin/pull/28289#pullrequestreview-1584012832)
ACK 239431444216850b63ecf01c3b5c5d6d24230d08
(https://github.com/bitcoin/bitcoin/pull/28289#pullrequestreview-1584012832)
ACK 239431444216850b63ecf01c3b5c5d6d24230d08
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1298184373)
heh, but I don't like templates either. My preference would be to use `if constexpr (is_pointer||is_optional) { foo } else { bar }` or C++20 concepts. See also https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293478970
However, I've taken your idea to remove ArgRef. Thanks!
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1298184373)
heh, but I don't like templates either. My preference would be to use `if constexpr (is_pointer||is_optional) { foo } else { bar }` or C++20 concepts. See also https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293478970
However, I've taken your idea to remove ArgRef. Thanks!
💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#issuecomment-1683590844)
> the ghcr can be used to store a ccache?
Anyway, it won't work with non-Linux runners out-of-the-box.
(https://github.com/bitcoin/bitcoin/pull/28292#issuecomment-1683590844)
> the ghcr can be used to store a ccache?
Anyway, it won't work with non-Linux runners out-of-the-box.
🚀 fanquake merged a pull request: "rpc: remove one more quote from non-string oneline description"
(https://github.com/bitcoin/bitcoin/pull/28289)
(https://github.com/bitcoin/bitcoin/pull/28289)
💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298220709)
> I mean, topic branches in contributors' fork repos.
The cache key isn't included the branch name, so I don't think this will work.
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298220709)
> I mean, topic branches in contributors' fork repos.
The cache key isn't included the branch name, so I don't think this will work.
💬 fanquake commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298222825)
Can someone elaborate on why branches in forks of this repository are something we need to consider when deciding on our caching stratergy for this repository? Do the forks share our cache (resources)? Can anyone with a fork clobber our cache?
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298222825)
Can someone elaborate on why branches in forks of this repository are something we need to consider when deciding on our caching stratergy for this repository? Do the forks share our cache (resources)? Can anyone with a fork clobber our cache?
💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298223512)
On a contributor's topic branch in his personal repo, the workflow will be triggered by a `push` event. So the updated push will reuse the cache.
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298223512)
On a contributor's topic branch in his personal repo, the workflow will be triggered by a `push` event. So the updated push will reuse the cache.
💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298225748)
> Can anyone with a fork clobber our cache?
No.
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298225748)
> Can anyone with a fork clobber our cache?
No.
💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298227841)
> Can someone elaborate on why branches in forks of this repository are something we need to consider when deciding on our caching stratergy for this repository?
I use CI in my personal repo before pushing my branch into the pull request. And I want to use CI with some efficiency.
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298227841)
> Can someone elaborate on why branches in forks of this repository are something we need to consider when deciding on our caching stratergy for this repository?
I use CI in my personal repo before pushing my branch into the pull request. And I want to use CI with some efficiency.
💬 MarcoFalke commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298227846)
> On a contributor's topic branch in their personal repo, the workflow will be triggered by a `push` event. So the updated push will reuse the cache.
No? I think it will use the latest cache, which may be from a different branch, because the cache key isn't included the branch name, so I don't think this will work.
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298227846)
> On a contributor's topic branch in their personal repo, the workflow will be triggered by a `push` event. So the updated push will reuse the cache.
No? I think it will use the latest cache, which may be from a different branch, because the cache key isn't included the branch name, so I don't think this will work.
💬 hebasto commented on pull request "ci: Disable cache save for pull requests in GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298233032)
> > On a contributor's topic branch in their personal repo, the workflow will be triggered by a `push` event. So the updated push will reuse the cache.
>
> No? I think it will use the latest cache, which may be from a different branch, because the cache key isn't included the branch name, so I don't think this will work.
https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#example-using-multiple-restore-keys: a feature branch is considered before a
...
(https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298233032)
> > On a contributor's topic branch in their personal repo, the workflow will be triggered by a `push` event. So the updated push will reuse the cache.
>
> No? I think it will use the latest cache, which may be from a different branch, because the cache key isn't included the branch name, so I don't think this will work.
https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#example-using-multiple-restore-keys: a feature branch is considered before a
...
💬 fanquake commented on pull request "guix: switch from `guix environment` to `guix shell`":
(https://github.com/bitcoin/bitcoin/pull/26077#issuecomment-1683642107)
> lgtm, but you'll have to document the minimum guix version to be 1.4, no?
Yea we'll have to update docs in some capacity. Going to draft for now, until we've bumped the time-machine (27897), and then come back to this.
(https://github.com/bitcoin/bitcoin/pull/26077#issuecomment-1683642107)
> lgtm, but you'll have to document the minimum guix version to be 1.4, no?
Yea we'll have to update docs in some capacity. Going to draft for now, until we've bumped the time-machine (27897), and then come back to this.
📝 fanquake converted_to_draft a pull request: "guix: switch from `guix environment` to `guix shell`"
(https://github.com/bitcoin/bitcoin/pull/26077)
See https://guix.gnu.org/manual/devel/en/html_node/Invoking-guix-environment.html.
> Deprecation warning: The guix environment command is deprecated
in favor of guix shell, which performs similar functions but is more convenient to use. See Invoking guix shell.
> Being deprecated, guix environment is slated for eventual removal,
but the Guix project is committed to keeping it until May 1st, 2023. Please get in touch with us at guix-devel@gnu.org if you would like to discuss it.
See al
...
(https://github.com/bitcoin/bitcoin/pull/26077)
See https://guix.gnu.org/manual/devel/en/html_node/Invoking-guix-environment.html.
> Deprecation warning: The guix environment command is deprecated
in favor of guix shell, which performs similar functions but is more convenient to use. See Invoking guix shell.
> Being deprecated, guix environment is slated for eventual removal,
but the Guix project is committed to keeping it until May 1st, 2023. Please get in touch with us at guix-devel@gnu.org if you would like to discuss it.
See al
...
🚀 fanquake merged a pull request: "crypto: more `Span<std::byte>` modernization & follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28100)
(https://github.com/bitcoin/bitcoin/pull/28100)
💬 MarcoFalke commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1683706701)
rebased (trivial)
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1683706701)
rebased (trivial)
💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1298294545)
@luke-jr like this?
> This will need `aRules` in `rpc/mining.cpp` updated
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1298294545)
@luke-jr like this?
> This will need `aRules` in `rpc/mining.cpp` updated
💬 BrandonOdiwuor commented on pull request "test: refactor: support sending funds with outpoint result":
(https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1298301189)
Review ACK 7154542
The proposed refactor is a commendable improvement, streamlining test cases by leveraging the ```create_outpoints(...)``` function to consolidate calls to ```nodes.sendtoaddress()``` and ```find_vout_for_address()``` into a single invocation.
Suggestion: It might be beneficial to add documentation clarifying that the ```outputs``` argument should be an array of dictionaries following the format ```{address: amount_to_send}```. This would enhance usability for future user
...
(https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1298301189)
Review ACK 7154542
The proposed refactor is a commendable improvement, streamlining test cases by leveraging the ```create_outpoints(...)``` function to consolidate calls to ```nodes.sendtoaddress()``` and ```find_vout_for_address()``` into a single invocation.
Suggestion: It might be beneficial to add documentation clarifying that the ```outputs``` argument should be an array of dictionaries following the format ```{address: amount_to_send}```. This would enhance usability for future user
...
🚀 fanquake merged a pull request: "refactor: Enforce C-str fmt strings in WalletLogPrintf()"
(https://github.com/bitcoin/bitcoin/pull/28237)
(https://github.com/bitcoin/bitcoin/pull/28237)
📝 fanquake unlocked a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217)
The default activation of the `permitbaremultisig=0` option proposes an enhancement for the Bitcoin network. By refusing non-P2SH multisignature transactions from the outset, this modification would contribute to reducing spam attempts and maintaining a healthy decentralization by discouraging undesirable activities.
(https://github.com/bitcoin/bitcoin/pull/28217)
The default activation of the `permitbaremultisig=0` option proposes an enhancement for the Bitcoin network. By refusing non-P2SH multisignature transactions from the outset, this modification would contribute to reducing spam attempts and maintaining a healthy decentralization by discouraging undesirable activities.
💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1298311222)
Oh wait, without `!`
I'll push a fix and add a test...
Before and after this change it should be:
```
bitcoin-cli getblocktemplate '{"rules": ["segwit"]}' | jq '.rules'
[
"csv",
"!segwit",
"taproot"
]
```
(https://github.com/bitcoin/bitcoin/pull/26201#discussion_r1298311222)
Oh wait, without `!`
I'll push a fix and add a test...
Before and after this change it should be:
```
bitcoin-cli getblocktemplate '{"rules": ["segwit"]}' | jq '.rules'
[
"csv",
"!segwit",
"taproot"
]
```
💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-1683759219)
Rebased after kernel changes. Fixed a regression in `getblocktemplate`'s `rules` result, and added a test for tit.
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-1683759219)
Rebased after kernel changes. Fixed a regression in `getblocktemplate`'s `rules` result, and added a test for tit.