💬 willcl-ark commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1210781782)
```suggestion
from test_framework.messages import CAddress, hash256
```

📝 furszy opened a pull request: "fuzz: fix wallet notifications.cpp"
(https://github.com/bitcoin/bitcoin/pull/27786)
Fixing https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1568815816.

As the fuzzer test requires all blocks to be scanned by the wallet
(because it is asserting the wallet balance at the end), we need to
ensure that no blocks are skipped by the recently added wallet
birth time functionality.

This just means setting the chain accumulated time to the maximum
value, so the wallet birth time is always below it, and the block is
always processed by the wallet.

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1569086614)
Thanks Marco. Fixed in #27786.
Would be nice if the CI alert us about this kind of stuff prior merging the PR.
The base fuzzer corpus should have detected it right?.

⚠️ Macoophonic opened an issue: "Adchoose"
(https://github.com/bitcoin/bitcoin/issues/27787)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

thuannguyen19992000@gmail.com

### Expected behaviour

Eppo

### Steps to reproduce

It idon's data no way of the world

### Relevant log output

......

### How did you obtain Bitcoin Core

Package manager

### What version of Bitcoin Core are you using?

Đọc.join.29.v

### Operating system and version

13.0.

### Machine specifications

_No response_

✅ willcl-ark closed an issue: "Adchoose"
(https://github.com/bitcoin/bitcoin/issues/27787)

:lock: fanquake locked an issue: "Adchoose"
(https://github.com/bitcoin/bitcoin/issues/27787)

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917)
Code review ACK 3c51e44380ba8041e5d6c4cb29b9b2c54fad0b4b

Overall changes look good, but I think two things could be done to make this easier to review:

1. The main thing would be making the "walletdb: refactor X loading" commits self contained and not leave behind dead code that needs to be cleaned up later. It's hard to identify what code is being replaced in current commits when not all the code being replaced is actually removed.
2. I think it might be good to move first 2 commits up t
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1204298801)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (422a8436089c844934903a61fcec6b7b93995c07)

If prefix being non-empty is a requirement, it would be good to add this `Assume` to the `MockableBatch` cursor as well, so all of the implementations of this method consistently reject empty prefixes, and test cases don't appear to work with the mock database and then fail with real databases.

Alternately you could consider just moving the `Assume` checks out of the GetNewPrefixCursor metho
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1204326544)
In commit "walletdb: Refactor key reading and loading to its own function" (82e025297ba8939660b15fad738be723d783165e)

Note for other reviewers: There appears to be a slight change of behavior because now `wss.nKeys` will be incremented if the public key is not valid. This doesn't actually change behavior because counts are not used if there are any failures:

https://github.com/bitcoin/bitcoin/blob/214f8f18b310e3af88eba6a005439ae423ccd76a/src/wallet/walletdb.cpp#L932-L933

Also `wss` vari
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1204957277)
In commit "walletdb: Refactor hd chain loading to its own function" (f079b7580d4e7fa0d373decd1ffd7144abb83bab)

Seems ok, but is there a rationale for previous commits catching exceptions in the refactored functions and this commit not catching exceptions?

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1205489696)
In commit "salvage: Remove use of ReadKeyValue in salvage" (e9d974b7f4136c04f35091438f5e78e17d77299b)

I guess this will no longer catch std::exception, but that is ok?

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1206093314)
In commit "walletdb: refactor tx loading" (ad3ae8b6f4b2f3430b9d6c1b5bcf2627be15d961)

It's confusing that this commit leaves behind dead code implementing the same logic as the new code. Would suggest getting rid of it:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -300,11 +300,8 @@ bool WalletBatch::EraseLockedUTXO(const COutPoint& output)

class CWalletScanState {
public:
- bool fAnyUnordered{false};
- std::vector<uint256> vWalletUpgrade;
st
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1205886280)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (b3425cc31feba14c5a0b0c4532437f12b5a901ff)

What its this change doing? It seems unrelated to the commit and maybe would be better as a separate commit with an explanation.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1206095209)
In commit "walletdb: Refactor descriptor wallet records loading" (51518daa60952f31c34013c643feb8fe11c9777e)

This commit is leaving behind dead code, including another copy of this print statement. Would be better to get rid of it here:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -310,11 +310,7 @@ public:
std::vector<uint256> vWalletUpgrade;
std::map<OutputType, uint256> m_active_external_spks;
std::map<OutputType, uint256> m_active_internal_s
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1210777446)
In commit "walletdb: refactor active spkm loading" (8a4362f9bcf213164f43395faa164e50aab9363b)

There's still some dead code left behind this commit:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -300,8 +300,6 @@ bool WalletBatch::EraseLockedUTXO(const COutPoint& output)

class CWalletScanState {
public:
- std::map<OutputType, uint256> m_active_external_spks;
- std::map<OutputType, uint256> m_active_internal_spks;

CWalletScanState() = defaul
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1206097033)
In commit "walletdb: Refactor wallet flags loading" (ede0c95237e47de8a93af229f33f0cbedf5780fd)

It seems good that this commit is changing the error code from CORRUPT to TOO_NEW when there are recognized flags, but it is confusing that this still leaves behind more code below that attempts to do this same thing. It would be good to remove that code as part of this commit:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -880,9 +880,6 @@ DBErrors WalletBatch::LoadWal
...

💬 theuni commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210830915)
I would expect to see `boost/multi_index_container.hpp` removed from this list as part of this PR, but it looks like it's still used:
```bash
$ git grep multi_index_container.hpp origin/pr/27783
origin/pr/27783:node/miner.h:#include <boost/multi_index_container.hpp>
origin/pr/27783:txmempool.h:#include <boost/multi_index_container.hpp>
origin/pr/27783:txrequest.cpp:#include <boost/multi_index_container.hpp>
```

Is there some complication with those 3 that prevents us from using the spe
...

💬 theuni commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210833326)
Nevermind, I misremembered how `multi_index_container.hpp` worked. I was thinking it was just a dumb list of other includes, but it's more than that.

Still, does using `multi_index_container_fwd.hpp` help at all?

📝 achow101 opened a pull request: "rpc: Be able to access RPC parameters by name"
(https://github.com/bitcoin/bitcoin/pull/27788)
Although we accept named RPC parameters, we still access the parameters by position within the RPC implementations. This PR makes it possible to access these parameters directly by name. This is achieved by holding the parameters in a separate `JSONRPCParameters` class which contains mappings from both name to value, and position to value. Two `operator[]` methods are implemented which can accept both strings and ints. This allows for backwards compatibility, while also allowing for future imple
...

💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1210967450)
In 411485082c22b86e1224f60534fccf1e2bb8e8f3 "RPC: Add add OBJ_NAMED_PARAMS type"

This seems somewhat fragile as it disallows having any arguments after the options object. While I would not expect that such arguments would be intentionally added to any RPC, it's possible that one could be added accidentally. Furthermore, the tests would not necessarily catch this issue if they do not use the named-only parameters.