💬 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.

💬 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);
```

💬 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()`.

💬 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
...

💬 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()`

💬 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
...

💬 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
```

💬 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
...

💬 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-
...

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1944189545)
> ```fish
> "walletversion": 169900,
> ```

Yes, HD wallets are fine. The issue is only about pre-HD wallets. My version is `60000`.

📝 maflcko opened a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434)
Passing large BTC/kvB feerates to RPCs is problematic, because:

* They are likely a typo. 1BTC/kvB (or larger) seems absurd.
* They cause signed integer overflow.

Fix all issues by rejecting anything more than 1BTC/kvB during parsing.

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#issuecomment-1944197968)
The following `FUZZ=rpc` should pass with this pull and crash on master:

```
c2VuZHJhd3RyYW5zYWN0aW9uXAEAAAAAHgBiu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7
u7u7u7u7u7u7u7u7u7u7u7u7u7u7u
...

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1489781074)
Updated, thanks!

🤔 pablomartin4btc reviewed a pull request: "Update translation source file for v27.0 string freeze"
(https://github.com/bitcoin-core/gui/pull/793#pullrequestreview-1880788565)
post-merge tACK 3d1bb1a122a037e966c2fb2e2113f0440edb8d93

no diff

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#issuecomment-1944215847)
`2e312ea909...808fe4ef80`: update comment, see https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1489333827

👍 vasild approved a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-1880798912)
ACK ba021087a2a8d1b5866460a386ea42e0b643c184

💬 sdaftuar commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1489790971)
> If `old`, `new`, and `tail` are CFeeFrac objects, this condition is exactly `!((new - old) << tail)`. If new is smaller than old, the `new - old` object has negative size.

@sipa With the various `Assume()` calls that check that if the size is 0, the fee must also be zero, doesn't that mean that we can't really write code like this example you gave? Unless we checked that new != old first -- otherwise you might create a FeeFrac with 0 size and non-zero fee.

⚠️ achow101 opened an issue: "wallet: Unrelated conflicted parent txs do not cause child txs to be marked as conflicted"
(https://github.com/bitcoin/bitcoin/issues/29435)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

If we receive a transaction which spends from an unconfirmed parent, and that parent is replaced, the child does not get marked as conflicted but rather only as inactive. If the child contains an input from our wallet, that input cannot be reused until the child is abandoned.

See also https://github.com/ACINQ/eclair/pull/2818

### Expected behaviour

We should try to detect if a such a
...

📝 brunoerg opened a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436)
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.

I did an experiment of calling `Select` without passing a network until it
...

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-1944409727)
cc: @vasild @naumenkogs @amitiuttarwar @mzumsande