💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2708437327)
Concept ACK.
I wrote two test cases to understand the general behavior of `InvalidateBlock`, not specifically to test this PR's changes:
```
A->B->C->D->E->F (main chain)
A->B->C->G (fork 1)
A->B->H->I->J (fork 2)
A->B->H->K (fork 3)
```
In the [first test](https://github.com/stringintech/bitcoin/blob/1b36dd6a66a44e8756d92e71baa7676646b5f1ce/src/test/validation_block_tests.cpp#L455), I invalidate block C (on active chain) which should invalidate C, D, E, F, and G. The [second test](h
...
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2708437327)
Concept ACK.
I wrote two test cases to understand the general behavior of `InvalidateBlock`, not specifically to test this PR's changes:
```
A->B->C->D->E->F (main chain)
A->B->C->G (fork 1)
A->B->H->I->J (fork 2)
A->B->H->K (fork 3)
```
In the [first test](https://github.com/stringintech/bitcoin/blob/1b36dd6a66a44e8756d92e71baa7676646b5f1ce/src/test/validation_block_tests.cpp#L455), I invalidate block C (on active chain) which should invalidate C, D, E, F, and G. The [second test](h
...
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1986138492)
Looking at the test_framework.py functions, they're about syncing *between* nodes. Maybe this one could be called `wait_on_txindex()`, but the RPC object uses "synced", so I'm not sure.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1986138492)
Looking at the test_framework.py functions, they're about syncing *between* nodes. Maybe this one could be called `wait_on_txindex()`, but the RPC object uses "synced", so I'm not sure.
💬 mabu44 commented on pull request "cmake: Check for `makensis` tool before using it":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2708461024)
tACK deea0988720aab65b3cc6d80b50bbf0d537adb24
Got same result as @hodlinator on Debian.
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2708461024)
tACK deea0988720aab65b3cc6d80b50bbf0d537adb24
Got same result as @hodlinator on Debian.
🤔 l0rinc reviewed a pull request: "doc: add missing copyright headers"
(https://github.com/bitcoin/bitcoin/pull/31864#pullrequestreview-2669260943)
Not entirely clear to me when which MIT license URL we're using and when we're using year ranges and when single years.
The changes since my last ack:
<details>
<summary>patch</summary>
```patch
diff --git a/src/clientversion.cpp b/src/clientversion.cpp
--- a/src/clientversion.cpp(revision f4fec329f5353d2b904659e129ade239fee09d89)
+++ b/src/clientversion.cpp(date 1741466438223)
@@ -96,6 +96,6 @@
"\n" +
"\n" +
_("This is experimental software
...
(https://github.com/bitcoin/bitcoin/pull/31864#pullrequestreview-2669260943)
Not entirely clear to me when which MIT license URL we're using and when we're using year ranges and when single years.
The changes since my last ack:
<details>
<summary>patch</summary>
```patch
diff --git a/src/clientversion.cpp b/src/clientversion.cpp
--- a/src/clientversion.cpp(revision f4fec329f5353d2b904659e129ade239fee09d89)
+++ b/src/clientversion.cpp(date 1741466438223)
@@ -96,6 +96,6 @@
"\n" +
"\n" +
_("This is experimental software
...
💬 l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1986151160)
Is there any reason for not updating these as well?
```suggestion
// file COPYING or https://opensource.org/license/MIT.
```
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1986151160)
Is there any reason for not updating these as well?
```suggestion
// file COPYING or https://opensource.org/license/MIT.
```
💬 l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1986151662)
I see you've removed the `2024-present` from the latest push - is that a general direction from now on?
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1986151662)
I see you've removed the `2024-present` from the latest push - is that a general direction from now on?
📝 hodlinator opened a pull request: "qa: Enable feature_init.py on Windows"
(https://github.com/bitcoin/bitcoin/pull/32021)
Windows has been skipped since feature_init.py was added in #23289. Possibly due to poorer support on older Python versions, or attempts to use `CTRL_C_EVENT` (which didn't work in my testing either) instead of `CTRL_BREAK_EVENT`.
(https://github.com/bitcoin/bitcoin/pull/32021)
Windows has been skipped since feature_init.py was added in #23289. Possibly due to poorer support on older Python versions, or attempts to use `CTRL_C_EVENT` (which didn't work in my testing either) instead of `CTRL_BREAK_EVENT`.
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1986159680)
As this thread prompted me to understand the beginning of the test better, I realized I could take a stab at making it work on Windows, and found a way: #32021.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1986159680)
As this thread prompted me to understand the beginning of the test better, I realized I could take a stab at making it work on Windows, and found a way: #32021.
💬 fjahr commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1986177762)
Right, I'm not passionate about the naming here. `sync_blocks` is syncing between two blockchains, `sync_txindex` is syncing between a blockchain and an index, so it's not wrong IMO. But `wait_for_txindex()` sounds fine, too. So up to you what you want to choose and if you want to change this at all.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1986177762)
Right, I'm not passionate about the naming here. `sync_blocks` is syncing between two blockchains, `sync_txindex` is syncing between a blockchain and an index, so it's not wrong IMO. But `wait_for_txindex()` sounds fine, too. So up to you what you want to choose and if you want to change this at all.
💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986292978)
not sure about default args. It would be better to explicitly enumerate the call sites and just put the default there, if needed.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986292978)
not sure about default args. It would be better to explicitly enumerate the call sites and just put the default there, if needed.
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1986316731)
I've always been a bit puzzled of this check. Is it really required? If so, why don't we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn't a fatal error be more appropriate then?
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1986316731)
I've always been a bit puzzled of this check. Is it really required? If so, why don't we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn't a fatal error be more appropriate then?
💬 hebasto commented on issue "build: ccache doesn't hit across build dirs":
(https://github.com/bitcoin/bitcoin/issues/31994#issuecomment-2708846512)
## TL;DR
To enable ccache hits across different build directories on Linux or macOS, configure the ccache options as follows:
```
base_dir = <path_to_your_home_directory>
hash_dir = false
```
or via environment variables:
```
$ export CCACHE_BASEDIR=$HOME
$ export CCACHE_NOHASHDIR=1
```
## Detailed Solution
All following commands were executed on Ubuntu 24.04.2 LTS with [ccache 4.9.1](https://packages.ubuntu.com/noble/ccache) installed.
Consider building in two separate build directories wit
...
(https://github.com/bitcoin/bitcoin/issues/31994#issuecomment-2708846512)
## TL;DR
To enable ccache hits across different build directories on Linux or macOS, configure the ccache options as follows:
```
base_dir = <path_to_your_home_directory>
hash_dir = false
```
or via environment variables:
```
$ export CCACHE_BASEDIR=$HOME
$ export CCACHE_NOHASHDIR=1
```
## Detailed Solution
All following commands were executed on Ubuntu 24.04.2 LTS with [ccache 4.9.1](https://packages.ubuntu.com/noble/ccache) installed.
Consider building in two separate build directories wit
...
💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2708847639)
> > I'm wondering in which scenarios a user might wish to disable the `-fdebug-prefix-map` flag?
>
> I don't think there are any. Just mentioned this because `-fdebug-prefix-map` seems to be added conditionally in our build.
Specifically, which condition are you referring to?
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2708847639)
> > I'm wondering in which scenarios a user might wish to disable the `-fdebug-prefix-map` flag?
>
> I don't think there are any. Just mentioned this because `-fdebug-prefix-map` seems to be added conditionally in our build.
Specifically, which condition are you referring to?
💬 hebasto commented on issue "build: ccache doesn't hit across build dirs":
(https://github.com/bitcoin/bitcoin/issues/31994#issuecomment-2708848882)
Amending our docs, as demonstrated in https://github.com/davidgumberg/bitcoin/commit/1c6ae1043d59d01cbf52f353e50a63b0afa883c4, could be an alternative to https://github.com/bitcoin/bitcoin/pull/30861.
(https://github.com/bitcoin/bitcoin/issues/31994#issuecomment-2708848882)
Amending our docs, as demonstrated in https://github.com/davidgumberg/bitcoin/commit/1c6ae1043d59d01cbf52f353e50a63b0afa883c4, could be an alternative to https://github.com/bitcoin/bitcoin/pull/30861.
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2708856306)
Thanks for the survey and summary, as well as the additional input on this topic!
I don't have a strong opinion on this, and anything is probably fine, as long as it "works".
Just some random thoughts:
* The GHA runners that are included for public repositories are likely too weak to run all of the CI tasks in the maximum timeout (apart from the cache being too small as well, as mentioned above). So the goal that more of the CI tasks can run on forks trivially may not be achievable.
* The 32-
...
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2708856306)
Thanks for the survey and summary, as well as the additional input on this topic!
I don't have a strong opinion on this, and anything is probably fine, as long as it "works".
Just some random thoughts:
* The GHA runners that are included for public repositories are likely too weak to run all of the CI tasks in the maximum timeout (apart from the cache being too small as well, as mentioned above). So the goal that more of the CI tasks can run on forks trivially may not be achievable.
* The 32-
...
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986324475)
@rkrux
Utilized existing `raw_multisig_transaction_legacy_tests` but with version 3 raw transaction. This seems to cover all the domain you specified. Could you pls take a look?
https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/test/functional/rpc_rawtransaction.py#L96
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986324475)
@rkrux
Utilized existing `raw_multisig_transaction_legacy_tests` but with version 3 raw transaction. This seems to cover all the domain you specified. Could you pls take a look?
https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/test/functional/rpc_rawtransaction.py#L96
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986325419)
Added variable to denote min, max value of the version.
https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/src/rpc/rawtransaction_util.cpp#L159-L160
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986325419)
Added variable to denote min, max value of the version.
https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/src/rpc/rawtransaction_util.cpp#L159-L160
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986325898)
@glozow
Are you implying that we should use the variable instead of constant values in the test code?
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986325898)
@glozow
Are you implying that we should use the variable instead of constant values in the test code?
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986326221)
@luke-jr @adamandrews1
So should I remove the line? If I should, could you advise which code should be updated to avoid failure of the aforementioned validation?
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986326221)
@luke-jr @adamandrews1
So should I remove the line? If I should, could you advise which code should be updated to avoid failure of the aforementioned validation?
💬 maflcko commented on pull request "qa: Enable feature_init.py on Windows":
(https://github.com/bitcoin/bitcoin/pull/32021#issuecomment-2708870591)
lgtm ACK 59c4930394cafc939eb396224b3d60d01ba0ce37
(https://github.com/bitcoin/bitcoin/pull/32021#issuecomment-2708870591)
lgtm ACK 59c4930394cafc939eb396224b3d60d01ba0ce37