💬 maflcko commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2866750733)
Anything left to be done here? It looks like the initial reports were about some `depends`-specific issues. However, the discussion then went into a general discussion about O3 and release notes? The `-DCMAKE_CXX_FLAGS_type` thing should be fixed by now.
It could make sense to file a new issue, or at least change the title to clarify that this is a depends-related report. Assuming that this isn't already covered by https://github.com/bitcoin/bitcoin/pull/31920 or https://github.com/bitcoin/bitc
...
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2866750733)
Anything left to be done here? It looks like the initial reports were about some `depends`-specific issues. However, the discussion then went into a general discussion about O3 and release notes? The `-DCMAKE_CXX_FLAGS_type` thing should be fixed by now.
It could make sense to file a new issue, or at least change the title to clarify that this is a depends-related report. Assuming that this isn't already covered by https://github.com/bitcoin/bitcoin/pull/31920 or https://github.com/bitcoin/bitc
...
💬 vasild commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2866750836)
`e83494d75f...63a14e293a`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2866750836)
`e83494d75f...63a14e293a`: rebase due to conflicts
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081798410)
Maybe; we never added the deprecation notice there, so i'm not convinced i should touch that now.
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081798410)
Maybe; we never added the deprecation notice there, so i'm not convinced i should touch that now.
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081801771)
Yes, this was and is still alllowed. Not sure it's a problem, if people want multiple passwords for one username they can do so, it doesn't open any security hole. In any case doesn't seem in scope of this change, which intentionally doesn't change user-visible behavior.
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081801771)
Yes, this was and is still alllowed. Not sure it's a problem, if people want multiple passwords for one username they can do so, it doesn't open any security hole. In any case doesn't seem in scope of this change, which intentionally doesn't change user-visible behavior.
💬 furszy commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2866761109)
> > Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
>
> AFAICT this will cause `importdescriptor` to throw an error with the message "Range should not be specified for an un-ranged descriptor"
Then how can the user trigger the error being solved here? If it can't be triggered by the user, we're talking about an extra guard rather than a fix — which is fine as well, but it would be better to label th
...
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2866761109)
> > Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
>
> AFAICT this will cause `importdescriptor` to throw an error with the message "Range should not be specified for an un-ranged descriptor"
Then how can the user trigger the error being solved here? If it can't be triggered by the user, we're talking about an extra guard rather than a fix — which is fine as well, but it would be better to label th
...
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081803994)
Agree, it's no longer possible to have `:` in `-rpcpassword` after this due to the choice to split them using `Split()`. I could change that code, though `:` in `-rpcuser` should really be invalid as the spec doesn't allow it.
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081803994)
Agree, it's no longer possible to have `:` in `-rpcpassword` after this due to the choice to split them using `Split()`. I could change that code, though `:` in `-rpcuser` should really be invalid as the spec doesn't allow it.
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081805880)
i did that initially but it was a much larger change than i wanted. It seems this works fine, with the only exception the `:`: but that gives a clear error at least.
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081805880)
i did that initially but it was a much larger change than i wanted. It seems this works fine, with the only exception the `:`: but that gives a clear error at least.
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081808045)
Agree.
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081808045)
Agree.
🤔 rkrux reviewed a pull request: "refactor: Removals after bdb removal"
(https://github.com/bitcoin/bitcoin/pull/32438#pullrequestreview-2828584996)
Makes sense (and prefer) to remove the dead code right after the removal PR is merged, helpful for future readers of the codebase. I have not checked the history around the watch-only stuff but rest of the diff makes sense to me. Build and tests pass on my system.
utACK fa061bf
(https://github.com/bitcoin/bitcoin/pull/32438#pullrequestreview-2828584996)
Makes sense (and prefer) to remove the dead code right after the removal PR is merged, helpful for future readers of the codebase. I have not checked the history around the watch-only stuff but rest of the diff makes sense to me. Build and tests pass on my system.
utACK fa061bf
📝 Sjors opened a pull request: "qt: drop unused watch-only functionality"
(https://github.com/bitcoin/bitcoin/pull/32459)
The watch-only functionality in the GUI was only used for legacy wallets. Watch-only descriptor wallets do not use this.
The only visible changes of this commit should be dropping the "Spendable:" label from the overview tab.
Builds on #32438.
(https://github.com/bitcoin/bitcoin/pull/32459)
The watch-only functionality in the GUI was only used for legacy wallets. Watch-only descriptor wallets do not use this.
The only visible changes of this commit should be dropping the "Spendable:" label from the overview tab.
Builds on #32438.
👍 theStack approved a pull request: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868#pullrequestreview-2828598770)
Light ACK 3a18075aedd7cff6f06b5fe10966d618b6378701
Cross-compiled for Windows from a Linux machine [using mingw-w64](https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/doc/build-windows.md?plain=1#L48-L52) and ran the involved unit and functional tests on a Windows 10 machine (needed to adapt the paths `./test/config.ini` manually on the target system, like also [done in the CI job](https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/
...
(https://github.com/bitcoin/bitcoin/pull/29868#pullrequestreview-2828598770)
Light ACK 3a18075aedd7cff6f06b5fe10966d618b6378701
Cross-compiled for Windows from a Linux machine [using mingw-w64](https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/doc/build-windows.md?plain=1#L48-L52) and ran the involved unit and functional tests on a Windows 10 machine (needed to adapt the paths `./test/config.ini` manually on the target system, like also [done in the CI job](https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/
...
💬 maflcko commented on pull request "qt: drop unused watch-only functionality":
(https://github.com/bitcoin/bitcoin/pull/32459#discussion_r2081826786)
this should probably be a separate commit, just updating the whole file, or nothing in the file?
(https://github.com/bitcoin/bitcoin/pull/32459#discussion_r2081826786)
this should probably be a separate commit, just updating the whole file, or nothing in the file?
💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081831829)
Anyhow, i'll cherry-pick this, thanks, hope i won't get the opposite comment now that this is changing too much 😓
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081831829)
Anyhow, i'll cherry-pick this, thanks, hope i won't get the opposite comment now that this is changing too much 😓
💬 hebasto commented on pull request "qt: drop unused watch-only functionality":
(https://github.com/bitcoin/bitcoin/pull/32459#discussion_r2081841029)
https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/src/qt/bitcoinstrings.cpp#L5
(https://github.com/bitcoin/bitcoin/pull/32459#discussion_r2081841029)
https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/src/qt/bitcoinstrings.cpp#L5
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2081860464)
added a comment and bumped the value
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2081860464)
added a comment and bumped the value
💬 rkrux commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2866886845)
> Then how can the user trigger the error being solved here? If it can't be triggered by the user, we're talking about an extra guard rather than a fix — which is fine as well, but it would be better to label the PR differently, since it won't need to be backported.
Agree that the PR can be labeled differently.
> When attempting to add a non-ranged descriptor with a start/end range of [0,0] via AddWalletDescriptor, an error may appear
The issue #31728 description states that it is trigg
...
(https://github.com/bitcoin/bitcoin/pull/32344#issuecomment-2866886845)
> Then how can the user trigger the error being solved here? If it can't be triggered by the user, we're talking about an extra guard rather than a fix — which is fine as well, but it would be better to label the PR differently, since it won't need to be backported.
Agree that the PR can be labeled differently.
> When attempting to add a non-ranged descriptor with a start/end range of [0,0] via AddWalletDescriptor, an error may appear
The issue #31728 description states that it is trigg
...
📝 fanquake opened a pull request: "fs: remove `_POSIX_C_SOURCE` defining"
(https://github.com/bitcoin/bitcoin/pull/32460)
From what I can tell, compilers on Linux systems, will be defining `_GNU_SOURCE`, which results in `glibc` defining `_POSIX_C_SOURCE` to `200809L`; so undefining it, and setting it to an earlier value does not seem like the correct behaviour for us, or even required, to check that this function is usable on Linux. I think there's also the chance that this could have adverse effects, by essentially trying to opt out of more mordern POSIX behaviour?
I think if anything, the project should be se
...
(https://github.com/bitcoin/bitcoin/pull/32460)
From what I can tell, compilers on Linux systems, will be defining `_GNU_SOURCE`, which results in `glibc` defining `_POSIX_C_SOURCE` to `200809L`; so undefining it, and setting it to an earlier value does not seem like the correct behaviour for us, or even required, to check that this function is usable on Linux. I think there's also the chance that this could have adverse effects, by essentially trying to opt out of more mordern POSIX behaviour?
I think if anything, the project should be se
...
✅ fanquake closed an issue: "cmake inconsistently overriding `-O3` (sometimes)"
(https://github.com/bitcoin/bitcoin/issues/31491)
(https://github.com/bitcoin/bitcoin/issues/31491)
💬 fanquake commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2866893762)
Given the amount of discussion here, we can re-open new issues.
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2866893762)
Given the amount of discussion here, we can re-open new issues.
⚠️ l0rinc opened an issue: "test: `acceptstalefeeestimates` failure in `feature_fee_estimation` after duplicate coinbase tx weight reservation fix"
(https://github.com/bitcoin/bitcoin/issues/32461)
### Summary
First encountered in https://github.com/bitcoin/bitcoin/actions/runs/14929268797/job/41941137828?pr=31144
Doing a git bisect reveals the likely culprit is [mining: bugfix: Fix duplicate coinbase tx weight reservation](https://github.com/bitcoin/bitcoin/pull/31384)
---
### Reproducer
> git checkout 6b165f5906fc53bd10bedff85a6ef26e0aabdc5c
```bash
HEAD is now at 6b165f5906 Merge bitcoin/bitcoin#31384: mining: bugfix: Fix duplicate coinbase tx weight reservation
```
> cmake -B bui
...
(https://github.com/bitcoin/bitcoin/issues/32461)
### Summary
First encountered in https://github.com/bitcoin/bitcoin/actions/runs/14929268797/job/41941137828?pr=31144
Doing a git bisect reveals the likely culprit is [mining: bugfix: Fix duplicate coinbase tx weight reservation](https://github.com/bitcoin/bitcoin/pull/31384)
---
### Reproducer
> git checkout 6b165f5906fc53bd10bedff85a6ef26e0aabdc5c
```bash
HEAD is now at 6b165f5906 Merge bitcoin/bitcoin#31384: mining: bugfix: Fix duplicate coinbase tx weight reservation
```
> cmake -B bui
...