💬 maflcko commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866555065)
> I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.
Yes, see https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2859123413
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866555065)
> I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.
Yes, see https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2859123413
💬 sipa commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2866560897)
Unsure if this matters in your case, but the cache being warm or not impacts validation speed significantly. If you have a node that is running for a long time, and then stop and start it again, the speed will be slower for a while until the cache warms up again.
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2866560897)
Unsure if this matters in your case, but the cache being warm or not impacts validation speed significantly. If you have a node that is running for a long time, and then stop and start it again, the speed will be slower for a while until the cache warms up again.
💬 fanquake commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2081724770)
> They were introduced in https://github.com/bitcoin/bitcoin/pull/28865/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248R45-R49,
> @fanquake, can you help us out here, do you remember why these were added?
You can recreate the failures by checking out to that commit (fd30e9688e15fe6e0f8b64202a6e9c7d96333394), applying this diff (which includes the supression being done here):
```diff
diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_f
...
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2081724770)
> They were introduced in https://github.com/bitcoin/bitcoin/pull/28865/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248R45-R49,
> @fanquake, can you help us out here, do you remember why these were added?
You can recreate the failures by checking out to that commit (fd30e9688e15fe6e0f8b64202a6e9c7d96333394), applying this diff (which includes the supression being done here):
```diff
diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_f
...
💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081728831)
Oh I see I misunderstood. For now I want LineReader to return new string objects because it reads directly from the HTTPClient's `m_recv_buffer` which is then erased. I know we can prevent that copy by being more clever with `m_recv_buffer` and I still plan on exploring that, maybe as a follow up? It's nice and simple now and there is a clean separation between the socket layer receiving data and the `HTTPRequest`
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081728831)
Oh I see I misunderstood. For now I want LineReader to return new string objects because it reads directly from the HTTPClient's `m_recv_buffer` which is then erased. I know we can prevent that copy by being more clever with `m_recv_buffer` and I still plan on exploring that, maybe as a follow up? It's nice and simple now and there is a clean separation between the socket layer receiving data and the `HTTPRequest`
💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081732772)
Sounds good, thanks for the context :)
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081732772)
Sounds good, thanks for the context :)
💬 fanquake commented on pull request "guix: move `*-check.py` scripts under contrib/guix/":
(https://github.com/bitcoin/bitcoin/pull/32458#issuecomment-2866617418)
Guix Build:
```bash
f15ff81485789fe4fa22c3ecf887cfbfa0b3ae8620fd8c8926deb257ef19cff9 guix-build-415650cea94f/output/aarch64-linux-gnu/SHA256SUMS.part
7320f11f0ff5005d841dc2d854f46c5d317df077dae41b4631493f090b34875c guix-build-415650cea94f/output/aarch64-linux-gnu/bitcoin-415650cea94f-aarch64-linux-gnu-debug.tar.gz
af696399978c60a9517006c8435d8053b541b3dcfec49dacaa46932404d22b7b guix-build-415650cea94f/output/aarch64-linux-gnu/bitcoin-415650cea94f-aarch64-linux-gnu.tar.gz
6da90889d6dcb600
...
(https://github.com/bitcoin/bitcoin/pull/32458#issuecomment-2866617418)
Guix Build:
```bash
f15ff81485789fe4fa22c3ecf887cfbfa0b3ae8620fd8c8926deb257ef19cff9 guix-build-415650cea94f/output/aarch64-linux-gnu/SHA256SUMS.part
7320f11f0ff5005d841dc2d854f46c5d317df077dae41b4631493f090b34875c guix-build-415650cea94f/output/aarch64-linux-gnu/bitcoin-415650cea94f-aarch64-linux-gnu-debug.tar.gz
af696399978c60a9517006c8435d8053b541b3dcfec49dacaa46932404d22b7b guix-build-415650cea94f/output/aarch64-linux-gnu/bitcoin-415650cea94f-aarch64-linux-gnu.tar.gz
6da90889d6dcb600
...
💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081736316)
Cool will address this nit on next rebase thanks!
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081736316)
Cool will address this nit on next rebase thanks!
💬 laanwj commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#issuecomment-2866738093)
> Is there a benchmark that needs this? If yes, going for synthetic, but representative (and easily adjustable) data may be a better choice for that benchmark.
Yes, as there is a lot of random data in a block whose exact value isn't important to benchmarking (only that it's always the same), it seems possible to deterministically construct a similar block from code.
(https://github.com/bitcoin/bitcoin/pull/32457#issuecomment-2866738093)
> Is there a benchmark that needs this? If yes, going for synthetic, but representative (and easily adjustable) data may be a better choice for that benchmark.
Yes, as there is a lot of random data in a block whose exact value isn't important to benchmarking (only that it's always the same), it seems possible to deterministically construct a similar block from code.
💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2866740624)
ACK 44298f0cb9c3de42b1c0f464c0d7b2779aa4c3b7
Using `ConsumeTransaction` works, and makes sense. Here's the [coverage](https://marcofleon.github.io/coverage/merkle/coverage/root/bitcoin/src/consensus/merkle.cpp.html). Everything lgtm!
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2866740624)
ACK 44298f0cb9c3de42b1c0f464c0d7b2779aa4c3b7
Using `ConsumeTransaction` works, and makes sense. Here's the [coverage](https://marcofleon.github.io/coverage/merkle/coverage/root/bitcoin/src/consensus/merkle.cpp.html). Everything lgtm!
👍 laanwj approved a pull request: "guix: move `*-check.py` scripts under contrib/guix/"
(https://github.com/bitcoin/bitcoin/pull/32458#pullrequestreview-2828547209)
Code review ACK 415650cea94f5050d7c368fdf9fd9878809957e1
(https://github.com/bitcoin/bitcoin/pull/32458#pullrequestreview-2828547209)
Code review ACK 415650cea94f5050d7c368fdf9fd9878809957e1
💬 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.