π¬ instagibbs commented on issue "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages":
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3271344694)
Chatting with some other devs, one other motivation for better/easier error codes would be to allow logging to be more sane.
For instance, `sendrawtransaction` may throw an error if already in mempool, which if logged is incredibly noisy and useless for fire-and-forget wallets like CLN.
submitpackage already behaves better in that respect, returning a success instead (as it is in the mempool).
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3271344694)
Chatting with some other devs, one other motivation for better/easier error codes would be to allow logging to be more sane.
For instance, `sendrawtransaction` may throw an error if already in mempool, which if logged is incredibly noisy and useless for fire-and-forget wallets like CLN.
submitpackage already behaves better in that respect, returning a success instead (as it is in the mempool).
π¬ Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3271397302)
Rebased. Fixed bug in the functional test found by an AI overlord, apparently using `is` isn't safe: https://stackoverflow.com/a/28864111
> I'll look into modifying miniscript / descriptor classes to avoid the string parsing.
Done, and also added a unit test. It's a bit more code and complexity, but more robust and extendable than parsing strings.
Since c1e6a9f1e7331fa2c1c73a0dd17056570283e443 is no longer relevant for this PR I dropped it.
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3271397302)
Rebased. Fixed bug in the functional test found by an AI overlord, apparently using `is` isn't safe: https://stackoverflow.com/a/28864111
> I'll look into modifying miniscript / descriptor classes to avoid the string parsing.
Done, and also added a unit test. It's a bit more code and complexity, but more robust and extendable than parsing strings.
Since c1e6a9f1e7331fa2c1c73a0dd17056570283e443 is no longer relevant for this PR I dropped it.
π¬ sipa commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3271403052)
utACK 417437eb01ac014c57aca47f44d7f8d3da351987
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3271403052)
utACK 417437eb01ac014c57aca47f44d7f8d3da351987
π¬ Sjors commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3271424867)
Oh oops...
```sh
bitcoin-cli gettxoutsetinfo muhash 913859
```
Both v29.1 and https://github.com/bitcoin/bitcoin/commit/c76797481155754329ec6a6f58e8402569043944:
```json
{
"height": 913859,
"bestblock": "00000000000000000000f7ae1989c423172433aab8aa0164f9fc85fd74ed7f44",
"txouts": 168005479,
"bogosize": 13163671088,
"muhash": "16bf45a4ce9c852aab746826955e3aac8e7e1b964a36af3bce944c17c5e13d55",
"total_amount": 19918082.42878334,
"total_unspendable_amount": 230.07121
...
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3271424867)
Oh oops...
```sh
bitcoin-cli gettxoutsetinfo muhash 913859
```
Both v29.1 and https://github.com/bitcoin/bitcoin/commit/c76797481155754329ec6a6f58e8402569043944:
```json
{
"height": 913859,
"bestblock": "00000000000000000000f7ae1989c423172433aab8aa0164f9fc85fd74ed7f44",
"txouts": 168005479,
"bogosize": 13163671088,
"muhash": "16bf45a4ce9c852aab746826955e3aac8e7e1b964a36af3bce944c17c5e13d55",
"total_amount": 19918082.42878334,
"total_unspendable_amount": 230.07121
...
π¬ theuni commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271448380)
Switching to `CMAKE_EXE_LINKER_FLAGS` came out of an offline discussion with fanquake.
@fanquake we want all flags passed to all binaries with the exception of `-static-libstdc++ -static-libgcc`, which should only go to exe's.
So I think we want:
`HOST_LDFLAGS="-Wl,--as-needed -Wl,--dynamic-linker=$glibc_dynamic_linker -Wl,-O2"`
`CMAKE_EXE_LINKER_FLAGS="-static-libstdc++ -static-libgcc"`
Then for CMake, still pass `LDFLAGS` via `HOST_LDFLAGS` in addition to `CMAKE_EXE_LINKER_FLAGS`. T
...
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271448380)
Switching to `CMAKE_EXE_LINKER_FLAGS` came out of an offline discussion with fanquake.
@fanquake we want all flags passed to all binaries with the exception of `-static-libstdc++ -static-libgcc`, which should only go to exe's.
So I think we want:
`HOST_LDFLAGS="-Wl,--as-needed -Wl,--dynamic-linker=$glibc_dynamic_linker -Wl,-O2"`
`CMAKE_EXE_LINKER_FLAGS="-static-libstdc++ -static-libgcc"`
Then for CMake, still pass `LDFLAGS` via `HOST_LDFLAGS` in addition to `CMAKE_EXE_LINKER_FLAGS`. T
...
π¬ fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271470246)
Yea, I need to rework this in any case, as the macOS build breaks with the changes here.
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3271470246)
Yea, I need to rework this in any case, as the macOS build breaks with the changes here.
π instagibbs approved a pull request: "Bump SCRIPT_VERIFY flags to 64 bit"
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3029931042)
ACK 417437eb01ac014c57aca47f44d7f8d3da351987
non-blocking suggestions for coverage
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3029931042)
ACK 417437eb01ac014c57aca47f44d7f8d3da351987
non-blocking suggestions for coverage
π¬ instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2213656693)
```Suggestion
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH), "P2SH");
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_TAPROOT), "P2SH,TAPROOT");
```
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2213656693)
```Suggestion
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH), "P2SH");
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_TAPROOT), "P2SH,TAPROOT");
```
π¬ instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2213603725)
bit of coverage of multiple unknown bits (didn't validate)
```Suggestion
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27)), "TAPROOT,0x08000000");
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27) | (1u<<31)), "TAPROOT,0x88000000");
```
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2213603725)
bit of coverage of multiple unknown bits (didn't validate)
```Suggestion
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27)), "TAPROOT,0x08000000");
BOOST_CHECK_EQUAL(FormatScriptFlags(SCRIPT_VERIFY_TAPROOT | (1u<<27) | (1u<<31)), "TAPROOT,0x88000000");
```
π¬ instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2334116135)
bddcadee82daf3ed1441820a0ffc4c5ef78f64f1
could be here or elsewhere, but round-simply round-tripping adds some coverage:
```
assert(script_verify_flags::from_int(flags.as_int()) == flags);
```
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2334116135)
bddcadee82daf3ed1441820a0ffc4c5ef78f64f1
could be here or elsewhere, but round-simply round-tripping adds some coverage:
```
assert(script_verify_flags::from_int(flags.as_int()) == flags);
```
π Crypt-iQ's pull request is ready for review: "fuzz: compact block harness"
(https://github.com/bitcoin/bitcoin/pull/33300)
(https://github.com/bitcoin/bitcoin/pull/33300)
π¬ Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3271483113)
There is non-determinism [here](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/node/blockstorage.cpp#L490-L493) because `m_dirty_blockindex` is `std::set<CBlockIndex*>` and sorts based on the pointer. This can be fixed by adding a function object that compares the block hashes. However, I did not know if this should be added just for fuzz code and I have not run benchmarks.
After patching the above non-determinism locally, there is also non-determinism in
...
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3271483113)
There is non-determinism [here](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/node/blockstorage.cpp#L490-L493) because `m_dirty_blockindex` is `std::set<CBlockIndex*>` and sorts based on the pointer. This can be fixed by adding a function object that compares the block hashes. However, I did not know if this should be added just for fuzz code and I have not run benchmarks.
After patching the above non-determinism locally, there is also non-determinism in
...
β οΈ darosior opened an issue: "Higher **reported** memory usage of Bitcoin Core after version 29"
(https://github.com/bitcoin/bitcoin/issues/33351)
For versions of Bitcoin Core after 29.0, common utilities like `top` will report higher RAM usage of the `bitcoind` process than for previous Bitcoin Core versions.
The size of LevelDB files was increased from 2 MiB to 32 MiB in Bitcoin Core 29.0 (PR https://github.com/bitcoin/bitcoin/pull/30039). LevelDB caches its on-disk files using [`mmap`](https://en.wikipedia.org/wiki/Mmap) and will cache up to 1000 files by default, although our fork increased this number to 4096 (see https://github.com/
...
(https://github.com/bitcoin/bitcoin/issues/33351)
For versions of Bitcoin Core after 29.0, common utilities like `top` will report higher RAM usage of the `bitcoind` process than for previous Bitcoin Core versions.
The size of LevelDB files was increased from 2 MiB to 32 MiB in Bitcoin Core 29.0 (PR https://github.com/bitcoin/bitcoin/pull/30039). LevelDB caches its on-disk files using [`mmap`](https://en.wikipedia.org/wiki/Mmap) and will cache up to 1000 files by default, although our fork increased this number to 4096 (see https://github.com/
...
π¬ BinaryIgor commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2334280301)
nit: typo, should be "validation"
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2334280301)
nit: typo, should be "validation"
π¬ BinaryIgor commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2334298532)
As this already is a rather too long test, it would be nice to move these assertions into a separate method, similarly as it done with check_tx_counts
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2334298532)
As this already is a rather too long test, it would be nice to move these assertions into a separate method, similarly as it done with check_tx_counts
β οΈ yancyribbens opened an issue: "CoinGrinder and SRD disagree on small change"
(https://github.com/bitcoin/bitcoin/issues/33352)
Both CoinGrinder and SRD create a change output, however they disagree on what change output is too small.
Consider SRD selection, which adds `CHANGE_LOWER` to the target value and in so doing, prevents a selection that is less than `CHANGE_LOWER` [ref](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/wallet/coinselection.cpp#L546C5-L546C47)
```
target_value += CHANGE_LOWER + change_fee;
```
However, CoinGrinder does not add `CHANGE_LOWER` [ref](https://gith
...
(https://github.com/bitcoin/bitcoin/issues/33352)
Both CoinGrinder and SRD create a change output, however they disagree on what change output is too small.
Consider SRD selection, which adds `CHANGE_LOWER` to the target value and in so doing, prevents a selection that is less than `CHANGE_LOWER` [ref](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/wallet/coinselection.cpp#L546C5-L546C47)
```
target_value += CHANGE_LOWER + change_fee;
```
However, CoinGrinder does not add `CHANGE_LOWER` [ref](https://gith
...
π¬ yancyribbens commented on issue "CoinGrinder and SRD disagree on small change":
(https://github.com/bitcoin/bitcoin/issues/33352#issuecomment-3271758012)
cc @murchandamus
(https://github.com/bitcoin/bitcoin/issues/33352#issuecomment-3271758012)
cc @murchandamus
π€ w0xlt reviewed a pull request: "test/refactor: use test deque to avoid quadratic iteration"
(https://github.com/bitcoin/bitcoin/pull/33313#pullrequestreview-3202749415)
ACK https://github.com/bitcoin/bitcoin/pull/33313/commits/8c1f10233dc9a843147772c40973031f5bfdbb7c
While the impact of this PR on a few hundred items is negligible, itβs still a good practice and is likely to scale more effectively as test coverage increases.
For reference, the script below demonstrates the performance difference with 1,000,000 items on my machine:
```
List pop(0): 62.790 seconds
Deque popleft(): 0.022 seconds
```
```python
import time
from collections import d
...
(https://github.com/bitcoin/bitcoin/pull/33313#pullrequestreview-3202749415)
ACK https://github.com/bitcoin/bitcoin/pull/33313/commits/8c1f10233dc9a843147772c40973031f5bfdbb7c
While the impact of this PR on a few hundred items is negligible, itβs still a good practice and is likely to scale more effectively as test coverage increases.
For reference, the script below demonstrates the performance difference with 1,000,000 items on my machine:
```
List pop(0): 62.790 seconds
Deque popleft(): 0.022 seconds
```
```python
import time
from collections import d
...
π¬ murchandamus commented on issue "CoinGrinder and SRD disagree on small change":
(https://github.com/bitcoin/bitcoin/issues/33352#issuecomment-3271804098)
This is intentional.
Knapsack and CoinGrinder use [`GenerateChangeTarget`](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/wallet/coinselection.cpp#L809) to make a [randomized minimum change](https://github.com/bitcoin/bitcoin/pull/24494), because these two algorithms minimize the overshoot over the minimum change which led to a fingerprint and a clustering of change outputs of the same value. SRD randomly selects inputs, so it already overshoots the minimum
...
(https://github.com/bitcoin/bitcoin/issues/33352#issuecomment-3271804098)
This is intentional.
Knapsack and CoinGrinder use [`GenerateChangeTarget`](https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/wallet/coinselection.cpp#L809) to make a [randomized minimum change](https://github.com/bitcoin/bitcoin/pull/24494), because these two algorithms minimize the overshoot over the minimum change which led to a fingerprint and a clustering of change outputs of the same value. SRD randomly selects inputs, so it already overshoots the minimum
...
π¬ davidgumberg commented on pull request "Avoid pathological QT text/markdown behavior...":
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3271811950)
> The patch indeed fixes the pathological memory usage. However, it modifies the clipboard content. The copy-pasted prompt appears as follow:
>
> [...]
Thanks for catching this, I should have checked that clipboard contents actually were reasonable with my change, my mistake.
I've updated the branch to fix both the newline issue, and the issues that existed with copying icon's and some of the other rich text stuff in the console. I use the same [function](https://github.com/qt/qtbase/bl
...
(https://github.com/bitcoin-core/gui/pull/886#issuecomment-3271811950)
> The patch indeed fixes the pathological memory usage. However, it modifies the clipboard content. The copy-pasted prompt appears as follow:
>
> [...]
Thanks for catching this, I should have checked that clipboard contents actually were reasonable with my change, my mistake.
I've updated the branch to fix both the newline issue, and the issues that existed with copying icon's and some of the other rich text stuff in the console. I use the same [function](https://github.com/qt/qtbase/bl
...