💬 virtu commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1943890840)
> There is the monitoring site at https://www.21.ninja/dns-seeds/ run by @virtu - maybe having some additional statistics about the diversity of results could have caught this issue?
Finally got around to this. I added two:
- The first attempts to measure diversity by counting the [distinct number of versions](https://21.ninja/dns-seeds/node-version-count/)
- The second is tailored specific for the issue at hand and shows the [version range](https://21.ninja/dns-seeds/node-version-range/)
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1943890840)
> There is the monitoring site at https://www.21.ninja/dns-seeds/ run by @virtu - maybe having some additional statistics about the diversity of results could have caught this issue?
Finally got around to this. I added two:
- The first attempts to measure diversity by counting the [distinct number of versions](https://21.ninja/dns-seeds/node-version-count/)
- The second is tailored specific for the issue at hand and shows the [version range](https://21.ninja/dns-seeds/node-version-range/)
💬 furszy commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1943916606)
Update: I'm cooking another set of improvements.. will try to have them for tonight.
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1943916606)
Update: I'm cooking another set of improvements.. will try to have them for tonight.
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1943928072)
Thank you.
I did a memtest86 check (the mix option), and a Prime95 stress test for about 24 hours in total. I encountered no issues.
CrystalDiskInfo has no critical warnings and gives Health Status as Good 99%.
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1943928072)
Thank you.
I did a memtest86 check (the mix option), and a Prime95 stress test for about 24 hours in total. I encountered no issues.
CrystalDiskInfo has no critical warnings and gives Health Status as Good 99%.
💬 pablomartin4btc commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1489587984)
When you run it, it will install an old version which doesn't work.
```
pip install transifex-client
Defaulting to user installation because normal site-packages is not writeable
Collecting transifex-client
Downloading transifex-client-0.12.5.tar.gz (181 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 181.5/181.5 KB 199.8 kB/s eta 0:00:00
Preparing metadata (setup.py) ... done
Requirement already satisfied: six in /usr/lib/python3/dist-packages (from transifex-client) (1.16.0)
R
...
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1489587984)
When you run it, it will install an old version which doesn't work.
```
pip install transifex-client
Defaulting to user installation because normal site-packages is not writeable
Collecting transifex-client
Downloading transifex-client-0.12.5.tar.gz (181 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 181.5/181.5 KB 199.8 kB/s eta 0:00:00
Preparing metadata (setup.py) ... done
Requirement already satisfied: six in /usr/lib/python3/dist-packages (from transifex-client) (1.16.0)
R
...
💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1489634608)
I've changed `resolvedAddresses` -> `connect_to`, and `resolvedAddr` -> `target_addr` for consistency
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1489634608)
I've changed `resolvedAddresses` -> `connect_to`, and `resolvedAddr` -> `target_addr` for consistency
💬 furszy commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944061663)
Or well, I just crafted a simple version of what I'm doing (because I'm planning to create a statement builder around this work, and that will take me some more time).
Could you please try this branch: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_insert and share the results?
It is essentially #28574 + 506b73872b9ce232e8c6b9fefbdb0c33e04d0707. The new commit introduces a raw multi-insert statement. Locally, this shows a significant speedup but I'm on a SSD.
...
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944061663)
Or well, I just crafted a simple version of what I'm doing (because I'm planning to create a statement builder around this work, and that will take me some more time).
Could you please try this branch: https://github.com/furszy/bitcoin-core/tree/2024_wallet_batch_migration_multi_insert and share the results?
It is essentially #28574 + 506b73872b9ce232e8c6b9fefbdb0c33e04d0707. The new commit introduces a raw multi-insert statement. Locally, this shows a significant speedup but I'm on a SSD.
...
💬 kristapsk commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1944067063)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1944067063)
Concept ACK
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1489710751)
For the time being I just added handling for `MSG_MORE` (on e.g. macOS sequential messages are sent separately while on Linux they're combined). I also made the timeouts a bit longer.
Hopefully that does the trick. This can be revisited closer to the time when the Template Provider is ready for its own PR.
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1489710751)
For the time being I just added handling for `MSG_MORE` (on e.g. macOS sequential messages are sent separately while on Linux they're combined). I also made the timeouts a bit longer.
Hopefully that does the trick. This can be revisited closer to the time when the Template Provider is ready for its own PR.
💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1489732482)
> What is the worst thing that can happen? A broken/malicious hostname could resolve to 100s addresses all of which timeout slowly, causing some delay here.
We are limiting the number of resolutions to 256
> I think that is ok, given that pzsDest never comes unchecked from random/evil peers over the P2P network and is always provided manually by the operator of the node, right?
That's my intuition too, yes. This is the call tree for `ConnectNode`:
```
RPC::AddConnection --> Add
...
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1489732482)
> What is the worst thing that can happen? A broken/malicious hostname could resolve to 100s addresses all of which timeout slowly, causing some delay here.
We are limiting the number of resolutions to 256
> I think that is ok, given that pzsDest never comes unchecked from random/evil peers over the P2P network and is always provided manually by the operator of the node, right?
That's my intuition too, yes. This is the call tree for `ConnectNode`:
```
RPC::AddConnection --> Add
...
🤔 vasild reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1879760071)
Approach ACK c02fd0379c425f486f1b80a81962c1aa68b8a852
Mostly minor stuff below plus a suggestion about the lifetime of the objects.
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1879760071)
Approach ACK c02fd0379c425f486f1b80a81962c1aa68b8a852
Mostly minor stuff below plus a suggestion about the lifetime of the objects.
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489303118)
nit: in commit message of a38dfb797f175aab45352f85fa80371861b22cd5 `[refactor] Prepare for g_signals de-globalization`:
`s/ValidationSignals/CMainSignals/`
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489303118)
nit: in commit message of a38dfb797f175aab45352f85fa80371861b22cd5 `[refactor] Prepare for g_signals de-globalization`:
`s/ValidationSignals/CMainSignals/`
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489155423)
The pointer can be `const` to hint the reader and the compiler that it will never start pointing to something else after it has been initialized in the constructor.
```suggestion
ValidationSignals* const m_signals;
```
For example constructs like:
```cpp
if (m_signals) {
...
m_signals->whatever();
}
```
are only safe because `m_signals` cannot change after the check.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489155423)
The pointer can be `const` to hint the reader and the compiler that it will never start pointing to something else after it has been initialized in the constructor.
```suggestion
ValidationSignals* const m_signals;
```
For example constructs like:
```cpp
if (m_signals) {
...
m_signals->whatever();
}
```
are only safe because `m_signals` cannot change after the check.
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489357931)
nit, feel free to ignore: maybe `uint256::ZERO` is clearer?
```suggestion
wallet.chain().waitForNotificationsIfTipChanged(uint256::ZERO);
```
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489357931)
nit, feel free to ignore: maybe `uint256::ZERO` is clearer?
```suggestion
wallet.chain().waitForNotificationsIfTipChanged(uint256::ZERO);
```
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489463065)
Elsewhere (including from `BaseIndex::Stop()`) there is a check whether `validation_signals` is null, which I think is missing here and in `BaseIndex::Init()`.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489463065)
Elsewhere (including from `BaseIndex::Stop()`) there is a check whether `validation_signals` is null, which I think is missing here and in `BaseIndex::Init()`.
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489457095)
Here a reference to `signals` is saved inside `NotificationsHandlerImpl` and is returned opaquely to the caller as a `Handler`. The caller has to make sure that the `Handler` they get does not live longer than `chainman().m_options.signals` but they have no idea about the latter. I understand that this is ok in the current code, but in general this is a recipe for disaster.
A cleaner and safer approach is to change the type of `ChainstateManagerOpts::signals` from `ValidationSignals*` to `std
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489457095)
Here a reference to `signals` is saved inside `NotificationsHandlerImpl` and is returned opaquely to the caller as a `Handler`. The caller has to make sure that the `Handler` they get does not live longer than `chainman().m_options.signals` but they have no idea about the latter. I understand that this is ok in the current code, but in general this is a recipe for disaster.
A cleaner and safer approach is to change the type of `ChainstateManagerOpts::signals` from `ValidationSignals*` to `std
...
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489594742)
Method names should begin with capital letter:
```suggestion
void SerialTaskRunner::Insert(std::function<void()> func)
```
Same for `SerialTaskRunner::flush()` and `SerialTaskRunner::size()`
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489594742)
Method names should begin with capital letter:
```suggestion
void SerialTaskRunner::Insert(std::function<void()> func)
```
Same for `SerialTaskRunner::flush()` and `SerialTaskRunner::size()`
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489527335)
Before we had a global `g_signals` and code that wished to access it called `GetMainSignals()`. This was safe wrt lifetime of the object.
With this PR it lives in `NodeContext::validation_signals` as `std::unique_ptr<ValidationSignals>`. Also (for covenience?) the object being pointed to by the `unique_ptr` is extracted from the `unique_ptr` and saved as a raw pointer in `ChainstateManager::Options::signals` and `CTxMemPool::Options::signals`. This defeats the idea of `unique_ptr` because aft
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489527335)
Before we had a global `g_signals` and code that wished to access it called `GetMainSignals()`. This was safe wrt lifetime of the object.
With this PR it lives in `NodeContext::validation_signals` as `std::unique_ptr<ValidationSignals>`. Also (for covenience?) the object being pointed to by the `unique_ptr` is extracted from the `unique_ptr` and saved as a raw pointer in `ChainstateManager::Options::signals` and `CTxMemPool::Options::signals`. This defeats the idea of `unique_ptr` because aft
...
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489711685)
I think the terms "subscriber" and "validation interface event" don't belong here.
```suggestion
* Execute a given task.
* The callback can either be queued for later/asynchronous/threaded
```
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489711685)
I think the terms "subscriber" and "validation interface event" don't belong here.
```suggestion
* Execute a given task.
* The callback can either be queued for later/asynchronous/threaded
```
💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489725106)
Doxygen attaches this comment to the `TaskRunnerInterface` class in the generated documentation which I guess is not the intention.
```suggestion
/** @file
*
* This header provides an interface and simple implementation for a task
* runner. Another threaded, serial implementation using a queue is available in
* the scheduler module's SerialTaskRunner.
*/
/**
* Interface for a task runner.
*/
class TaskRunnerInterface
```
See https://www.doxygen.nl/manual/commands.html#c
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489725106)
Doxygen attaches this comment to the `TaskRunnerInterface` class in the generated documentation which I guess is not the intention.
```suggestion
/** @file
*
* This header provides an interface and simple implementation for a task
* runner. Another threaded, serial implementation using a queue is available in
* the scheduler module's SerialTaskRunner.
*/
/**
* Interface for a task runner.
*/
class TaskRunnerInterface
```
See https://www.doxygen.nl/manual/commands.html#c
...
💬 willcl-ark commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944173949)
I tried to reproduce using a wallet with ~1k tx located on a spinning disk -- although this time on regtest -- but I found `migratewallet` to be pleasantly fast:
```fish
will@will-ubuntu in ~/src/bitcoin on master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
₿ time /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original migratewallet big
{
"wallet_name": "big",
"backup_path": "/media/will/PEPSI/big-wallet-900-tx-original/regtest/wallets/big/big-
...
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944173949)
I tried to reproduce using a wallet with ~1k tx located on a spinning disk -- although this time on regtest -- but I found `migratewallet` to be pleasantly fast:
```fish
will@will-ubuntu in ~/src/bitcoin on master [$?⇡] : C v17.0.2-clang : 🐍 3.9.18
₿ time /home/will/src/bitcoin/src/bitcoin-cli -regtest -datadir=/media/will/PEPSI/big-wallet-900-tx-original migratewallet big
{
"wallet_name": "big",
"backup_path": "/media/will/PEPSI/big-wallet-900-tx-original/regtest/wallets/big/big-
...