💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2284184677)
@fanquake [wrote](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777):
> I think we can improve the output when `-DWITH_CCACHE=OFF` is used. Depending on the system, that output might be:
>
> ```shell
> cmake -B build -DWITH_CCACHE=OFF
> < snip >
> Use ccache for compiling .............. ccache masquerades as the compiler
> ```
>
> We should probably at least indicate that the option was respected by the build-system.
The issue, along with another bug I've notice
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2284184677)
@fanquake [wrote](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777):
> I think we can improve the output when `-DWITH_CCACHE=OFF` is used. Depending on the system, that output might be:
>
> ```shell
> cmake -B build -DWITH_CCACHE=OFF
> < snip >
> Use ccache for compiling .............. ccache masquerades as the compiler
> ```
>
> We should probably at least indicate that the option was respected by the build-system.
The issue, along with another bug I've notice
...
🤔 fjahr reviewed a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2233277476)
Code review ACK 2e9072c137e81c75c58d0c0788295c10fdafdc9b
This is a good regression test to have. I left a bunch of minor nits but this is fine to merge as-is so feel free to leave them unless you have to re-touch.
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2233277476)
Code review ACK 2e9072c137e81c75c58d0c0788295c10fdafdc9b
This is a good regression test to have. I left a bunch of minor nits but this is fine to merge as-is so feel free to leave them unless you have to re-touch.
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713929119)
nit
```suggestion
assert_equal(scan_result['bestblock'], snapshot_hash)
```
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713929119)
nit
```suggestion
assert_equal(scan_result['bestblock'], snapshot_hash)
```
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713932403)
nit: Would be a bit more readable IMO if you would break it into two lines
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713932403)
nit: Would be a bit more readable IMO if you would break it into two lines
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713927674)
nit: could have moved this up and used it in the `gettxoutsetinfo` as well
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713927674)
nit: could have moved this up and used it in the `gettxoutsetinfo` as well
💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713931438)
On second thought: maybe just pull all three of these loaded keys out in the beginning since you use each of them at least twice and they are used again further below... or you could deduplicate the `assert_equal` triplet but it's not a big deal either way...
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1713931438)
On second thought: maybe just pull all three of these loaded keys out in the beginning since you use each of them at least twice and they are used again further below... or you could deduplicate the `assert_equal` triplet but it's not a big deal either way...
🚀 glozow merged a pull request: "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files"
(https://github.com/bitcoin/bitcoin/pull/30265)
(https://github.com/bitcoin/bitcoin/pull/30265)
💬 mzumsande commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2284290341)
> Concept ACK. if you retouch, there's also:
Good catch, fixed!
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2284290341)
> Concept ACK. if you retouch, there's also:
Good catch, fixed!
🤔 furszy reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2231855105)
Few comments. Will continue reviewing.
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2231855105)
Few comments. Will continue reviewing.
💬 furszy commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713021114)
I don't think showing the already migrated wallets here adds any value. The user cannot do anything with them and they do not present any extra information. I would skip them:
```diff
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -460,6 +460,10 @@
m_migrate_wallet_menu->clear();
for (const auto& [wallet_name, info] : m_wallet_controller->listWalletDir()) {
const auto& [loade
...
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713021114)
I don't think showing the already migrated wallets here adds any value. The user cannot do anything with them and they do not present any extra information. I would skip them:
```diff
diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
--- a/src/qt/bitcoingui.cpp
+++ b/src/qt/bitcoingui.cpp
@@ -460,6 +460,10 @@
m_migrate_wallet_menu->clear();
for (const auto& [wallet_name, info] : m_wallet_controller->listWalletDir()) {
const auto& [loade
...
💬 furszy commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713981246)
We don't actually need to do this. Could cherry-pick https://github.com/furszy/bitcoin-core/commit/f5c0c58977a8989fe17db361d179281d051b0806 and remove the introduced `getWallet`.
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713981246)
We don't actually need to do this. Could cherry-pick https://github.com/furszy/bitcoin-core/commit/f5c0c58977a8989fe17db361d179281d051b0806 and remove the introduced `getWallet`.
💬 furszy commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713845934)
Compilation error: `error: 'wallet_name' in capture list does not name a variable`.
It seems the lambda expression cannot capture the structured binding. A local reference of the variable fixes it.
Same happens on the `m_open_wallet_menu` action lambda with the "path" variable.
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1713845934)
Compilation error: `error: 'wallet_name' in capture list does not name a variable`.
It seems the lambda expression cannot capture the structured binding. A local reference of the variable fixes it.
Same happens on the `m_open_wallet_menu` action lambda with the "path" variable.
💬 Sjors commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2284311578)
`<datadir>/node.sock` seems fine, although on macOS with the longest permitted username you would go over the limit: `/Users/willem-alexanderclausgeorgeferdinand-konder-der-nederlanden-jonkheer-van-amsberg-67/Library/Application\ Support/Bitcoin/testnet4/bitcoin-node.sock`
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2284311578)
`<datadir>/node.sock` seems fine, although on macOS with the longest permitted username you would go over the limit: `/Users/willem-alexanderclausgeorgeferdinand-konder-der-nederlanden-jonkheer-van-amsberg-67/Library/Application\ Support/Bitcoin/testnet4/bitcoin-node.sock`
📝 paplorinc opened a pull request: "refactor: Migrate EmplaceCoinInternalDANGER to try_emplace"
(https://github.com/bitcoin/bitcoin/pull/30637)
Leftover from https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1652853326:
[`try_emplace`](https://en.cppreference.com/w/cpp/container/map/try_emplace) doc states
> 2) Behaves like emplace except that the element is constructed as
value_type(std::piecewise_construct,
std::forward_as_tuple(std::move(k)),
std::forward_as_tuple(std::forward<Args>(args)...))
---
The [`DANGER` part](https://github.com/bitcoin/bitcoin/pull/19806/commits/f6e2da5fb7c6406c37612
...
(https://github.com/bitcoin/bitcoin/pull/30637)
Leftover from https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1652853326:
[`try_emplace`](https://en.cppreference.com/w/cpp/container/map/try_emplace) doc states
> 2) Behaves like emplace except that the element is constructed as
value_type(std::piecewise_construct,
std::forward_as_tuple(std::move(k)),
std::forward_as_tuple(std::forward<Args>(args)...))
---
The [`DANGER` part](https://github.com/bitcoin/bitcoin/pull/19806/commits/f6e2da5fb7c6406c37612
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1714018553)
Opened https://github.com/bitcoin/bitcoin/pull/30637
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1714018553)
Opened https://github.com/bitcoin/bitcoin/pull/30637
💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2284321835)
Actually, looks like the random_max_len (which could be small enough) strategy is enabled 15% of the time in the OSS-Fuzz config anyway.
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2284321835)
Actually, looks like the random_max_len (which could be small enough) strategy is enabled 15% of the time in the OSS-Fuzz config anyway.
💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2284323856)
https://github.com/google/clusterfuzz/blob/578b401b655e95211b6ba9b805986832622b213f/src/clusterfuzz/_internal/fuzzing/strategy.py#L35
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2284323856)
https://github.com/google/clusterfuzz/blob/578b401b655e95211b6ba9b805986832622b213f/src/clusterfuzz/_internal/fuzzing/strategy.py#L35
💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2284331725)
.... though that's a random uniform value between 1 and 10000, so at 15% (which is of runs?) I'm not sure how often you'll ever see any target hit with a particularly _small_ max_len. it'll happen, but perhaps quite infrequently if there are targets that benefit substantially from small string runs
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2284331725)
.... though that's a random uniform value between 1 and 10000, so at 15% (which is of runs?) I'm not sure how often you'll ever see any target hit with a particularly _small_ max_len. it'll happen, but perhaps quite infrequently if there are targets that benefit substantially from small string runs
🤔 ryanofsky reviewed a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2233198217)
Code review dcf984b055a367facf7704eb8055619d6fe64a55. I think there might be an infinite loop but if waitfornewblock is called with timeout of 0, and it looks like there are some other quirks. Left a few suggestions below.
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2233198217)
Code review dcf984b055a367facf7704eb8055619d6fe64a55. I think there might be an infinite loop but if waitfornewblock is called with timeout of 0, and it looks like there are some other quirks. Left a few suggestions below.
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713960772)
In commit "Add waitTipChanged to Mining interface" (3435d7dd430559abffc964c163a2ae73febd99d1)
I think it would be slightly better for `current_tip` to just be a block hash, rather than hash and height, because the height value is unused, and a accepting height might create misleading impression that this method could be called to wait for a height to be reached.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713960772)
In commit "Add waitTipChanged to Mining interface" (3435d7dd430559abffc964c163a2ae73febd99d1)
I think it would be slightly better for `current_tip` to just be a block hash, rather than hash and height, because the height value is unused, and a accepting height might create misleading impression that this method could be called to wait for a height to be reached.