🤔 shaavan reviewed a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-2104235508)
ACK 659663af32f02c570e334e8f375fd5f5737258d7 ⚡️
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-2104235508)
ACK 659663af32f02c570e334e8f375fd5f5737258d7 ⚡️
📝 Eunovo opened a pull request: "Tr partial descriptors"
(https://github.com/bitcoin/bitcoin/pull/30243)
This PR adds support for rawnode() and rawleaf() descriptors discussed in #24114 and [tr-rawnode-and-rawleaf](https://delvingbitcoin.org/t/tr-rawnode-and-rawleaf-support/901)
(https://github.com/bitcoin/bitcoin/pull/30243)
This PR adds support for rawnode() and rawleaf() descriptors discussed in #24114 and [tr-rawnode-and-rawleaf](https://delvingbitcoin.org/t/tr-rawnode-and-rawleaf-support/901)
📝 Eunovo converted_to_draft a pull request: "Tr partial descriptors"
(https://github.com/bitcoin/bitcoin/pull/30243)
This PR adds support for rawnode() and rawleaf() under tr() descriptor discussed in #24114 and [tr-rawnode-and-rawleaf](https://delvingbitcoin.org/t/tr-rawnode-and-rawleaf-support/901)
(https://github.com/bitcoin/bitcoin/pull/30243)
This PR adds support for rawnode() and rawleaf() under tr() descriptor discussed in #24114 and [tr-rawnode-and-rawleaf](https://delvingbitcoin.org/t/tr-rawnode-and-rawleaf-support/901)
💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2154691207)
Thank you for your suggestions @ryanofsky! I applied both of your suggested patches.
eeea0818c1a20adc5225b98b185953d386c033e0 -> 682f1f1827dff78208bc1272f82d217c42a2cd69 ([preserveIndexOnRestart_4](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_4) -> [preserveIndexOnRestart_5](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/preserveIndexOnRestart_4..preserveIndexOnRestart_5))
* Addressed
...
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2154691207)
Thank you for your suggestions @ryanofsky! I applied both of your suggested patches.
eeea0818c1a20adc5225b98b185953d386c033e0 -> 682f1f1827dff78208bc1272f82d217c42a2cd69 ([preserveIndexOnRestart_4](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_4) -> [preserveIndexOnRestart_5](https://github.com/TheCharlatan/bitcoin/tree/preserveIndexOnRestart_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/preserveIndexOnRestart_4..preserveIndexOnRestart_5))
* Addressed
...
👍 hebasto approved a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2104389645)
re-ACK 7b8eea067f188c0b0e52ef21b01aedd37667a237, I've verified changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2103018546) with
```
git range-diff master ee253ca7dea9bed01d4c1800760477ef06310df8 7b8eea067f188c0b0e52ef21b01aedd37667a237
```
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2104389645)
re-ACK 7b8eea067f188c0b0e52ef21b01aedd37667a237, I've verified changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2103018546) with
```
git range-diff master ee253ca7dea9bed01d4c1800760477ef06310df8 7b8eea067f188c0b0e52ef21b01aedd37667a237
```
📝 m3dwards opened a pull request: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244)
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6, feature_proxy.py"'
```
(https://github.com/bitcoin/bitcoin/pull/30244)
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6, feature_proxy.py"'
```
💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2154727588)
reACK https://github.com/bitcoin/bitcoin/pull/30161/commits/7b8eea067f188c0b0e52ef21b01aedd37667a237
reviewed via `git range-diff master ee253ca 7b8eea0`
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2154727588)
reACK https://github.com/bitcoin/bitcoin/pull/30161/commits/7b8eea067f188c0b0e52ef21b01aedd37667a237
reviewed via `git range-diff master ee253ca 7b8eea0`
💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631127412)
I don't think this works, does it? installation happens when the image is built (`docker build`), not when the tests are compiled and run.
See the Cirrus CI comment:
```
# In the image build step, no external environment variables are available,
# so any settings will need to be written to the settings env file:
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631127412)
I don't think this works, does it? installation happens when the image is built (`docker build`), not when the tests are compiled and run.
See the Cirrus CI comment:
```
# In the image build step, no external environment variables are available,
# so any settings will need to be written to the settings env file:
🤔 maflcko requested changes to a pull request: "ci: move ASAN job to GitHub Actions from Cirrus CI"
(https://github.com/bitcoin/bitcoin/pull/30193#pullrequestreview-2104414380)
lgtm, except for the feedback
(https://github.com/bitcoin/bitcoin/pull/30193#pullrequestreview-2104414380)
lgtm, except for the feedback
💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631125417)
Please clarify in the name that this was set up for CI
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631125417)
Please clarify in the name that this was set up for CI
💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631127593)
?
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631127593)
?
💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2154745368)
> `git ls-files` is also used in `./test/lint/lint-python.py`, for reference. So using `git ls-files -- '*.md'` and passing the full array to mlc may be the best approach for us?
Thanks, I changed the approach in https://github.com/willcl-ark/mlc/tree/git-aware to use `git status --ignored *.md` and append to the ignored paths, as adding to that felt like the path of least resistance.
I will investigate if I can have a `--git` flag only check the result of `git ls-files -- '*.md'`, as I fe
...
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2154745368)
> `git ls-files` is also used in `./test/lint/lint-python.py`, for reference. So using `git ls-files -- '*.md'` and passing the full array to mlc may be the best approach for us?
Thanks, I changed the approach in https://github.com/willcl-ark/mlc/tree/git-aware to use `git status --ignored *.md` and append to the ignored paths, as adding to that felt like the path of least resistance.
I will investigate if I can have a `--git` flag only check the result of `git ls-files -- '*.md'`, as I fe
...
💬 maflcko commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631137020)
Also, it could split this up into a separate commit/pull, as this affects more than just this one task?
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631137020)
Also, it could split this up into a separate commit/pull, as this affects more than just this one task?
🤔 theStack reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2104427047)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2104427047)
Concept ACK
💬 theStack commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1631133040)
in commit f732495191d346b877ffe8402ec68761713eaaf7: now that the legacy-wallet-check is done without loading the wallet, is the check for the descriptor flag below still needed? (can a BDB wallet ever have this flag set?)
```
// Before anything else, check if there is something to migrate.
if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
if (was_loaded) {
reload_wallet(local_wallet);
}
return util::Error{_("Error: This wallet is
...
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1631133040)
in commit f732495191d346b877ffe8402ec68761713eaaf7: now that the legacy-wallet-check is done without loading the wallet, is the check for the descriptor flag below still needed? (can a BDB wallet ever have this flag set?)
```
// Before anything else, check if there is something to migrate.
if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
if (was_loaded) {
reload_wallet(local_wallet);
}
return util::Error{_("Error: This wallet is
...
📝 m3dwards opened a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245)
This is similar to (but does not fix) https://github.com/bitcoin/bitcoin/issues/13155 which I believe is the same issue but in libevent.
The issue is on a host that has IPV6 enabled but only a loopback IP address `-proxy=[::1]` will fail as `[::1]` is not considered valid by `getaddrinfo` with `AI_ADDRCONFIG` flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: `feature_proxy.py`.
To replicate the issue, run `feature_prox
...
(https://github.com/bitcoin/bitcoin/pull/30245)
This is similar to (but does not fix) https://github.com/bitcoin/bitcoin/issues/13155 which I believe is the same issue but in libevent.
The issue is on a host that has IPV6 enabled but only a loopback IP address `-proxy=[::1]` will fail as `[::1]` is not considered valid by `getaddrinfo` with `AI_ADDRCONFIG` flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: `feature_proxy.py`.
To replicate the issue, run `feature_prox
...
💬 m3dwards commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631165232)
I was under the impression that these things were not relevant for the test but if they are I will add them back in.
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1631165232)
I was under the impression that these things were not relevant for the test but if they are I will add them back in.
📝 virtu opened a pull request: "contrib: asmap-tool - Compare ASMaps with respect to specific addresses"
(https://github.com/bitcoin/bitcoin/pull/30246)
Right now, we have no way to quantify the "degradation" of an ASMap over time in the context of Bitcoin's P2P network in a meaningful way. However, such data would be useful for:
1. Making sure the minimum shelf life of ASMaps is compatible with the release cycle (we wouldn't want to start shipping ASMaps with releases before making sure ASMaps typically do not become obsolete before the time of the next release)
2. Node operators eager to keep their ASMaps up-to-date between releases.
Whil
...
(https://github.com/bitcoin/bitcoin/pull/30246)
Right now, we have no way to quantify the "degradation" of an ASMap over time in the context of Bitcoin's P2P network in a meaningful way. However, such data would be useful for:
1. Making sure the minimum shelf life of ASMaps is compatible with the release cycle (we wouldn't want to start shipping ASMaps with releases before making sure ASMaps typically do not become obsolete before the time of the next release)
2. Node operators eager to keep their ASMaps up-to-date between releases.
Whil
...
👍 maflcko approved a pull request: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244#pullrequestreview-2104485950)
I can't review this, because it is written in bash, but testing seems to indicate that this works as intended for some reason.
ACK b9c0b79ab7a0d4dafd5386a78f4fd747236c8f88
(https://github.com/bitcoin/bitcoin/pull/30244#pullrequestreview-2104485950)
I can't review this, because it is written in bash, but testing seems to indicate that this works as intended for some reason.
ACK b9c0b79ab7a0d4dafd5386a78f4fd747236c8f88
💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#discussion_r1631166504)
Is this still needed?
(https://github.com/bitcoin/bitcoin/pull/30244#discussion_r1631166504)
Is this still needed?