💬 luke-jr commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3130694316)
Seems like it would be better to split it into two, so the quicker part can be run by test_runner every time (without running it twice when the long reorg test is also desired)
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3130694316)
Seems like it would be better to split it into two, so the quicker part can be run by test_runner every time (without running it twice when the long reorg test is also desired)
💬 ajtowns commented on pull request "refactor: Move `FreespaceChecker` class into its own module":
(https://github.com/bitcoin-core/gui/pull/881#issuecomment-3130725962)
ACK dd392a64bb0608847f771f8b1f09c2fcae146923
Shouldn't the copyright dates in these files be updated? `git blame` has some lines from 2022 in intro.h and 2024/25 in intro.cpp.
You could avoid the circularity with something like:
```diff
diff --git a/src/qt/freespacechecker.h b/src/qt/freespacechecker.h
index d3a61a11571..214324be7c8 100644
--- a/src/qt/freespacechecker.h
+++ b/src/qt/freespacechecker.h
@@ -9,8 +9,6 @@
#include <QString>
#include <QtGlobal>
-class Intro;
-
...
(https://github.com/bitcoin-core/gui/pull/881#issuecomment-3130725962)
ACK dd392a64bb0608847f771f8b1f09c2fcae146923
Shouldn't the copyright dates in these files be updated? `git blame` has some lines from 2022 in intro.h and 2024/25 in intro.cpp.
You could avoid the circularity with something like:
```diff
diff --git a/src/qt/freespacechecker.h b/src/qt/freespacechecker.h
index d3a61a11571..214324be7c8 100644
--- a/src/qt/freespacechecker.h
+++ b/src/qt/freespacechecker.h
@@ -9,8 +9,6 @@
#include <QString>
#include <QtGlobal>
-class Intro;
-
...
💬 luke-jr commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3130771830)
Concept NACK, Bitcoin invoice addresses are single-use, and it doesn't make sense to put them in DNS.
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3130771830)
Concept NACK, Bitcoin invoice addresses are single-use, and it doesn't make sense to put them in DNS.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2238645631)
nit (from the llm):
PACAKGES -> PACKAGES [env var name typo]
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2238645631)
nit (from the llm):
PACAKGES -> PACKAGES [env var name typo]
💬 maflcko commented on issue "build: Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls":
(https://github.com/bitcoin/bitcoin/issues/33080#issuecomment-3130890341)
Pretty sure this was already fixed upstream in GCC stdlib ? What are the exact steps to reproduce and what is your GCC version? It does not happen in the CI, looking at https://github.com/bitcoin/bitcoin/actions/runs/16582003563/job/46899945634?pr=32989
(https://github.com/bitcoin/bitcoin/issues/33080#issuecomment-3130890341)
Pretty sure this was already fixed upstream in GCC stdlib ? What are the exact steps to reproduce and what is your GCC version? It does not happen in the CI, looking at https://github.com/bitcoin/bitcoin/actions/runs/16582003563/job/46899945634?pr=32989
💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3130944764)
The other CI failure looks interesting. I wonder if it can be fixed by replacing the hard-coded `CLI_MAX_ARG_SIZE` with `python3 -c 'import os; print(os.sysconf("SC_ARG_MAX"))'`
(https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3130944764)
The other CI failure looks interesting. I wonder if it can be fixed by replacing the hard-coded `CLI_MAX_ARG_SIZE` with `python3 -c 'import os; print(os.sysconf("SC_ARG_MAX"))'`
💬 maflcko commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238733043)
This is racy and will fail CI. You can reproduce this by adding a `time.sleep(9)` before the `with`?
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238733043)
This is racy and will fail CI. You can reproduce this by adding a `time.sleep(9)` before the `with`?
💬 l0rinc commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238737896)
Thanks, was wondering why it's failing on some platforms. Do you have a better alternative?
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238737896)
Thanks, was wondering why it's failing on some platforms. Do you have a better alternative?
💬 maflcko commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3130976782)
The remaining ones don't speed up IBD, do they?
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3130976782)
The remaining ones don't speed up IBD, do they?
💬 maflcko commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238745438)
The `with` scope will have to be increased, it you want to keep it
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238745438)
The `with` scope will have to be increased, it you want to keep it
💬 l0rinc commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3130990941)
The big ones are merged, thanks for your help.
These are smaller ones - at least from an IBD perspective, but both are quite simple.
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3130990941)
The big ones are merged, thanks for your help.
These are smaller ones - at least from an IBD perspective, but both are quite simple.
💬 l0rinc commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238768642)
Makes sense, thanks for the pointer!
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238768642)
Makes sense, thanks for the pointer!
⚠️ Sjors opened an issue: "wallet: control which taproot script path to spend"
(https://github.com/bitcoin/bitcoin/issues/33084)
When there's multiple script paths we can spend, the wallet just (partially) signs all of them. We then pick the one with the lowest fees:
https://github.com/bitcoin/bitcoin/blob/321984705dbcc5982013c8cfb52389cf3f2e75f6/src/script/sign.cpp#L373-L389
The user may want to override this, e.g.:
1. For privacy reasons they don't wish to reveal the less secure (but cheaper) fallback condition.
2. Their co-signer software trips up if it sees something in the `taproot_script_path_sigs` it can't han
...
(https://github.com/bitcoin/bitcoin/issues/33084)
When there's multiple script paths we can spend, the wallet just (partially) signs all of them. We then pick the one with the lowest fees:
https://github.com/bitcoin/bitcoin/blob/321984705dbcc5982013c8cfb52389cf3f2e75f6/src/script/sign.cpp#L373-L389
The user may want to override this, e.g.:
1. For privacy reasons they don't wish to reveal the less secure (but cheaper) fallback condition.
2. Their co-signer software trips up if it sees something in the `taproot_script_path_sigs` it can't han
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3131141240)
Updated 4eb1c66dbdaf35cbc480cb201f6019bc0f5fde95 -> 3d6b3fd8f65e8d4ea2c26d61c3dee89f5da10fac ([kernelApi_47](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_47) -> [kernelApi_48](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_48), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_47..kernelApi_48))
* Added test for failed block and undo data reads.
* Migrated unit tests to use our usual boost test framework. This was done initially to demonstrate that indeed
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3131141240)
Updated 4eb1c66dbdaf35cbc480cb201f6019bc0f5fde95 -> 3d6b3fd8f65e8d4ea2c26d61c3dee89f5da10fac ([kernelApi_47](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_47) -> [kernelApi_48](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_48), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_47..kernelApi_48))
* Added test for failed block and undo data reads.
* Migrated unit tests to use our usual boost test framework. This was done initially to demonstrate that indeed
...
📝 BrandonOdiwuor opened a pull request: "doc: gen-manpages.py should check build options "
(https://github.com/bitcoin/bitcoin/pull/33085)
Fixes https://github.com/bitcoin/bitcoin/issues/17506. Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/29457 (closed)
This PR fixes the `gen-manpages.py` script to check the build options and exit when there are missing components. It also adds an option to `--skip-build-options-check`
The PR also addresses the issues raised on PR: https://github.com/bitcoin/bitcoin/pull/29457:
- https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836669430 - the default behavi
...
(https://github.com/bitcoin/bitcoin/pull/33085)
Fixes https://github.com/bitcoin/bitcoin/issues/17506. Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/29457 (closed)
This PR fixes the `gen-manpages.py` script to check the build options and exit when there are missing components. It also adds an option to `--skip-build-options-check`
The PR also addresses the issues raised on PR: https://github.com/bitcoin/bitcoin/pull/29457:
- https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836669430 - the default behavi
...
💬 hebasto commented on pull request "Move `FreespaceChecker` class into its own module":
(https://github.com/bitcoin-core/gui/pull/881#issuecomment-3131240655)
@ajtowns
Thank you for the review! Your feedback has been addressed.
(https://github.com/bitcoin-core/gui/pull/881#issuecomment-3131240655)
@ajtowns
Thank you for the review! Your feedback has been addressed.
📝 deadmanoz opened a pull request: "contrib: [tracing] fix pointer argument handling in mempool_monitor.py"
(https://github.com/bitcoin/bitcoin/pull/33086)
The BPF code was incorrectly passing pointer variables by value to `bpf_usdt_readarg()`, causing the function to fail silently and resulting in transaction hashes showing as zeros and reason strings displaying empty strings.
This fix adds the missing reference operator (&) when passing pointer variables to `bpf_usdt_readarg()`, allowing the function to properly write the pointer values and enabling correct display of transaction hashes and removal/rejection reasons.
Fixes the regression in
...
(https://github.com/bitcoin/bitcoin/pull/33086)
The BPF code was incorrectly passing pointer variables by value to `bpf_usdt_readarg()`, causing the function to fail silently and resulting in transaction hashes showing as zeros and reason strings displaying empty strings.
This fix adds the missing reference operator (&) when passing pointer variables to `bpf_usdt_readarg()`, allowing the function to properly write the pointer values and enabling correct display of transaction hashes and removal/rejection reasons.
Fixes the regression in
...
💬 1440000bytes commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3131364440)
Concept NACK
- Adds unnecessary complexity
- Adds attack vector
- It is possible to achieve this functionality without changing bitcoin core
BIP 353 itself is controversial because it expects an average user to securely manage DNS for bitcoin payments.
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3131364440)
Concept NACK
- Adds unnecessary complexity
- Adds attack vector
- It is possible to achieve this functionality without changing bitcoin core
BIP 353 itself is controversial because it expects an average user to securely manage DNS for bitcoin payments.
🚀 fanquake merged a pull request: "validation: docs and cleanups for MemPoolAccept coins views"
(https://github.com/bitcoin/bitcoin/pull/32973)
(https://github.com/bitcoin/bitcoin/pull/32973)
✅ fanquake closed a pull request: "[NO MERGE]: TSAN should fail"
(https://github.com/bitcoin/bitcoin/pull/33081)
(https://github.com/bitcoin/bitcoin/pull/33081)