💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2012965947)
Two calls to `chainStateFlushed` are left, in `MigrateLegacyToDescriptor` and `bench/wallet_migration.cpp`
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2012965947)
Two calls to `chainStateFlushed` are left, in `MigrateLegacyToDescriptor` and `bench/wallet_migration.cpp`
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2752629027)
@stickies-v found an interesting example of a library, [SFML](https://www.sfml-dev.org/), that ships C++ headers from its "core" [repository](https://github.com/SFML/SFML), but also hosts C bindings in [another repository](https://github.com/SFML/CSFML/tree/master) in their organisation. They have a list of all known bindings to their library on their website: https://www.sfml-dev.org/download/bindings. I did a tour of some of them and most seem to be using the C bindings, but interestingly ther
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2752629027)
@stickies-v found an interesting example of a library, [SFML](https://www.sfml-dev.org/), that ships C++ headers from its "core" [repository](https://github.com/SFML/SFML), but also hosts C bindings in [another repository](https://github.com/SFML/CSFML/tree/master) in their organisation. They have a list of all known bindings to their library on their website: https://www.sfml-dev.org/download/bindings. I did a tour of some of them and most seem to be using the C bindings, but interestingly ther
...
💬 l0rinc commented on pull request "fuzz: extract unsequenced operations with side-effects":
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752631896)
> In any case, I don't see why this should be bundled in this pull request, which is a bugfix that probably should be backported.
It's not that important, I dropped that commit
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2752631896)
> In any case, I don't see why this should be bundled in this pull request, which is a bugfix that probably should be backported.
It's not that important, I dropped that commit
👍 hodlinator approved a pull request: "fuzz: extract unsequenced operations with side-effects"
(https://github.com/bitcoin/bitcoin/pull/32141#pullrequestreview-2715349904)
ACK b1de59e8965354fff5a149bc0fe61ed0704aea7a
Verified fix works on Windows/MSVC. Undid the change, and re-ran again to confirm that I could reproduce the failing assert issue due to unspecified evaluation order seen on CI.
#### PR description suggestion
```diff
- introduced an unsequenced operations with side-effects
+ introduced two unsequenced operations where one had side-effects
```
(https://github.com/bitcoin/bitcoin/pull/32141#pullrequestreview-2715349904)
ACK b1de59e8965354fff5a149bc0fe61ed0704aea7a
Verified fix works on Windows/MSVC. Undid the change, and re-ran again to confirm that I could reproduce the failing assert issue due to unspecified evaluation order seen on CI.
#### PR description suggestion
```diff
- introduced an unsequenced operations with side-effects
+ introduced two unsequenced operations where one had side-effects
```
💬 darosior commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2752699096)
> These are two different tests now
No they are not. This just made the existing test deterministic, of sorts.
> the old test is testing a very simple off by one error
To be honest i don't think the test as it stands today in `master` is all that useful. It claims to be testing a case where the coins count is too large. A too large coins count would "overflow" reading coins into the next coin's txid. In all likelihood this should fail to deserialize and return a ["Bad snapshot format or
...
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2752699096)
> These are two different tests now
No they are not. This just made the existing test deterministic, of sorts.
> the old test is testing a very simple off by one error
To be honest i don't think the test as it stands today in `master` is all that useful. It claims to be testing a case where the coins count is too large. A too large coins count would "overflow" reading coins into the next coin's txid. In all likelihood this should fail to deserialize and return a ["Bad snapshot format or
...
💬 darosior commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#discussion_r2013032671)
Will do if this gets more Concept ACKs.
(https://github.com/bitcoin/bitcoin/pull/32117#discussion_r2013032671)
Will do if this gets more Concept ACKs.
💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2013038501)
The 'trigger' transaction moves from the Vault state to the Unvaulting state.
- a Vault UTXO has no embedded data, so the internal key equals what I called the _naked key_ (which is the `alternate_pk` in this case).
- an Unvaulting UTXO has some data embedded (the withdrawal_pk), so the internal key is something like `tweak(alternate_pk, SHA256(alternate_pk || data))`. So if you put `-1` to send to an output with the "same public key as the input's internal key", you'd have the tweaked key as
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2013038501)
The 'trigger' transaction moves from the Vault state to the Unvaulting state.
- a Vault UTXO has no embedded data, so the internal key equals what I called the _naked key_ (which is the `alternate_pk` in this case).
- an Unvaulting UTXO has some data embedded (the withdrawal_pk), so the internal key is something like `tweak(alternate_pk, SHA256(alternate_pk || data))`. So if you put `-1` to send to an output with the "same public key as the input's internal key", you'd have the tweaked key as
...
💬 fjahr commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2752729648)
> But it is not true that all those tests are similarly brittle.
You are looking at these tests in isolation, that's not what I was talking about. The offsets of these test cases are still just as brittle and this carries through all the tests that would follow a mismatch. For example the coins count is a `VARINT` so it can have a different size and if its size of that `VARINT` changes in the dump then all the following offsets are wrong. All I am saying is that the test still has very strong
...
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2752729648)
> But it is not true that all those tests are similarly brittle.
You are looking at these tests in isolation, that's not what I was talking about. The offsets of these test cases are still just as brittle and this carries through all the tests that would follow a mismatch. For example the coins count is a `VARINT` so it can have a different size and if its size of that `VARINT` changes in the dump then all the following offsets are wrong. All I am saying is that the test still has very strong
...
⚠️ oren-z0 opened an issue: "Feature Request: testmempoolaccept with an argument to ignore nLocktime/nSequence time-locking"
(https://github.com/bitcoin/bitcoin/issues/32142)
### Please describe the feature you'd like to see added.
Execute `testmempoolaccept` in a way that will ignore the time/block-height validations of the transactions' `nSequence` and `nLocktime`.
### Is your feature related to a problem, if so please describe it.
`testmempoolaccept` is a very useful tool to verify transactions that aren't supposed to be broadcasted immediately - for example, for emergency recovery in case the seed is lost.
`nLocktime` is also useful in such case, to prevent th
...
(https://github.com/bitcoin/bitcoin/issues/32142)
### Please describe the feature you'd like to see added.
Execute `testmempoolaccept` in a way that will ignore the time/block-height validations of the transactions' `nSequence` and `nLocktime`.
### Is your feature related to a problem, if so please describe it.
`testmempoolaccept` is a very useful tool to verify transactions that aren't supposed to be broadcasted immediately - for example, for emergency recovery in case the seed is lost.
`nLocktime` is also useful in such case, to prevent th
...
📝 cculianu opened a pull request: "Fix 11-year-old mis-categorized error code in OP_IF evaluation"
(https://github.com/bitcoin/bitcoin/pull/32143)
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
(https://github.com/bitcoin/bitcoin/pull/32143)
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
:lock: achow101 locked an issue: "Consideration of a Code of Conduct and/or written rules for moderation"
(https://github.com/bitcoin/bitcoin/issues/29507)
(https://github.com/bitcoin/bitcoin/issues/29507)
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2013342912)
> and provided a working proof of concept that addresses #30206.
Thanks, however I tested that with GCC removed, and it didn't work?
(from the new commit)
> Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see #30451).
What is suboptimal? Can you post a build log, or something that actually shows what doesn't work?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2013342912)
> and provided a working proof of concept that addresses #30206.
Thanks, however I tested that with GCC removed, and it didn't work?
(from the new commit)
> Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see #30451).
What is suboptimal? Can you post a build log, or something that actually shows what doesn't work?
💬 laanwj commented on pull request "contrib: Add external RPC code generator with unit tests":
(https://github.com/bitcoin/bitcoin/pull/32140#issuecomment-2753185417)
`/schema.json` probably wasn't intended to be in this PR.
(https://github.com/bitcoin/bitcoin/pull/32140#issuecomment-2753185417)
`/schema.json` probably wasn't intended to be in this PR.
📝 strmfos opened a pull request: "Remove needless borrow to fix Clippy warning"
(https://github.com/bitcoin/bitcoin/pull/32144)
<!--
Pull Request Description
**Summary**
Removes a needless borrow in `test/lint/test_runner/src/main.rs` that triggered a
Clippy warning (`needless_borrows_for_generic_args`). This minor refactoring
makes the code cleaner without changing functionality.
**Rationale**
- Eliminates a Clippy warning when running:
```bash
cargo clippy --manifest-path test/lint/test_runner/Cargo.toml -- -D warnings
(https://github.com/bitcoin/bitcoin/pull/32144)
<!--
Pull Request Description
**Summary**
Removes a needless borrow in `test/lint/test_runner/src/main.rs` that triggered a
Clippy warning (`needless_borrows_for_generic_args`). This minor refactoring
makes the code cleaner without changing functionality.
**Rationale**
- Eliminates a Clippy warning when running:
```bash
cargo clippy --manifest-path test/lint/test_runner/Cargo.toml -- -D warnings
💬 strmfos commented on pull request "Remove needless borrow to fix Clippy warning":
(https://github.com/bitcoin/bitcoin/pull/32144#issuecomment-2753430170)
**Summary**
Removes a needless borrow in `test/lint/test_runner/src/main.rs` that triggered a
Clippy warning (`needless_borrows_for_generic_args`). This minor refactoring
makes the code cleaner without changing functionality.
**Rationale**
- Eliminates a Clippy warning when running:
```bash
cargo clippy --manifest-path test/lint/test_runner/Cargo.toml -- -D warnings
(https://github.com/bitcoin/bitcoin/pull/32144#issuecomment-2753430170)
**Summary**
Removes a needless borrow in `test/lint/test_runner/src/main.rs` that triggered a
Clippy warning (`needless_borrows_for_generic_args`). This minor refactoring
makes the code cleaner without changing functionality.
**Rationale**
- Eliminates a Clippy warning when running:
```bash
cargo clippy --manifest-path test/lint/test_runner/Cargo.toml -- -D warnings
💬 maflcko commented on pull request "lint: Remove needless borrow to fix Clippy warning":
(https://github.com/bitcoin/bitcoin/pull/32144#issuecomment-2753473305)
lgtm ACK e3ce2bd9829bbe14e5da26505ac9d68ae0d2af2d
(https://github.com/bitcoin/bitcoin/pull/32144#issuecomment-2753473305)
lgtm ACK e3ce2bd9829bbe14e5da26505ac9d68ae0d2af2d
💬 maflcko commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2753521601)
I missed when the rc bins were put up on the website, and it seems others have, too: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-03-20#1103484
I know it isn't documented in the release-process, but it could make sense to announce them on the mailing list, or here in this issue, to possibly get more testing by users.
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2753521601)
I missed when the rc bins were put up on the website, and it seems others have, too: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-03-20#1103484
I know it isn't documented in the release-process, but it could make sense to announce them on the mailing list, or here in this issue, to possibly get more testing by users.
💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2013583565)
Thanks, that makes sense. There's no concept of a global naked key that is preserved between steps of the program (except through the workaround you mention).
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2013583565)
Thanks, that makes sense. There's no concept of a global naked key that is preserved between steps of the program (except through the workaround you mention).
💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2013597677)
> Is the reason maybe that this is trying to optimize code and avoid needing to access the `g_enable_dynamic_fuzz_determinism` variable in release builds?
Yes, that's the reason as I understand it. I am happy to re-ack a simple doc fixup like:
```cpp
return false; // Allow optimized code for release builds (non-fuzz and non-debug).
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2013597677)
> Is the reason maybe that this is trying to optimize code and avoid needing to access the `g_enable_dynamic_fuzz_determinism` variable in release builds?
Yes, that's the reason as I understand it. I am happy to re-ack a simple doc fixup like:
```cpp
return false; // Allow optimized code for release builds (non-fuzz and non-debug).
💬 l0rinc commented on issue "RFC: Macro Regression Test Suite for Historical Reorgs":
(https://github.com/bitcoin/bitcoin/issues/32130#issuecomment-2753561878)
> you suggest is to simulate a situation where the syncing node thinks it is out of IBD to replay these reorgs
Exactly!
> historical reorgs weren't particularly deep or complicated
That's my understanding as well, but they're real, so they don't include our testing biases.
(https://github.com/bitcoin/bitcoin/issues/32130#issuecomment-2753561878)
> you suggest is to simulate a situation where the syncing node thinks it is out of IBD to replay these reorgs
Exactly!
> historical reorgs weren't particularly deep or complicated
That's my understanding as well, but they're real, so they don't include our testing biases.