💬 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.
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417)
In commit "Add waitTipChanged to Mining interface" (3435d7dd430559abffc964c163a2ae73febd99d1)
> I changed this in the last push, previously it would always wait for one second each interrupt "round" even if the requested `timeout` was shorter.
This doesn't seem right. If timeout is 1500ms and the block never changes, it looks like this will wait 1000ms+1000ms instead of 1000ms+500ms. I'm also concerned about the `deadline = now() + timeout` assignment above because it seems like that will
...
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417)
In commit "Add waitTipChanged to Mining interface" (3435d7dd430559abffc964c163a2ae73febd99d1)
> I changed this in the last push, previously it would always wait for one second each interrupt "round" even if the requested `timeout` was shorter.
This doesn't seem right. If timeout is 1500ms and the block never changes, it looks like this will wait 1000ms+1000ms instead of 1000ms+500ms. I'm also concerned about the `deadline = now() + timeout` assignment above because it seems like that will
...
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713965000)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (d919a230563bd2efee1fccb83194b43f436781ef)
It seems like the implementation of the this loop always waits for the timeout to be reached, and doesn't exit early if the tip did change. So if timeout is 0 it will just infinite loop.
I think if it can be simplified to just:
```c++
if (IsRPCRunning()) {
block = timeout
? miner.waitTipChanged(block, std::chrono::milliseconds(timeout))
: min
...
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713965000)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (d919a230563bd2efee1fccb83194b43f436781ef)
It seems like the implementation of the this loop always waits for the timeout to be reached, and doesn't exit early if the tip did change. So if timeout is 0 it will just infinite loop.
I think if it can be simplified to just:
```c++
if (IsRPCRunning()) {
block = timeout
? miner.waitTipChanged(block, std::chrono::milliseconds(timeout))
: min
...
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713997278)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (d919a230563bd2efee1fccb83194b43f436781ef)
Passing timeout parameter doesn't seem right, because it could wait too long. For example if timeout is 10 seconds, and tip changed after 8 seconds but did not match expected block hash, the next wait call will wait 10 seconds instead of 2 seconds.
Might be better to replace with:
```c++
while (IsRPCRunning()) {
auto now = timeout ? std::chrono::steady_clock::now() : std::chro
...
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713997278)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (d919a230563bd2efee1fccb83194b43f436781ef)
Passing timeout parameter doesn't seem right, because it could wait too long. For example if timeout is 10 seconds, and tip changed after 8 seconds but did not match expected block hash, the next wait call will wait 10 seconds instead of 2 seconds.
Might be better to replace with:
```c++
while (IsRPCRunning()) {
auto now = timeout ? std::chrono::steady_clock::now() : std::chro
...
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1714002168)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (d919a230563bd2efee1fccb83194b43f436781ef)
Seems like this has same problem as waitforblock where waitTipChanged could wait too long because it is called with wrong timeout value. Might be better as:
```c++
while (IsRPCRunning()) {
auto now = timeout ? std::chrono::steady_clock::now() : std::chrono::steady_clock::time_point::min();
if (block.height >= height || now > deadline) break;
block = timeout ? miner.wait
...
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1714002168)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (d919a230563bd2efee1fccb83194b43f436781ef)
Seems like this has same problem as waitforblock where waitTipChanged could wait too long because it is called with wrong timeout value. Might be better as:
```c++
while (IsRPCRunning()) {
auto now = timeout ? std::chrono::steady_clock::now() : std::chrono::steady_clock::time_point::min();
if (block.height >= height || now > deadline) break;
block = timeout ? miner.wait
...
💬 hebasto commented on pull request "Drop log category in `SeedStartup`":
(https://github.com/bitcoin/bitcoin/pull/29480#issuecomment-2284374843)
I won’t be working on this PR in the near future. So, closing it and labeling it "Up for grabs".
(https://github.com/bitcoin/bitcoin/pull/29480#issuecomment-2284374843)
I won’t be working on this PR in the near future. So, closing it and labeling it "Up for grabs".