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

💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569335260)
ACK 2808c33bae95a0d2516a6e9a550032af142a04de

I find `translateNamedArguments` somewhat difficult to read, but it seems like there isn't really much that can be done to make it easier to comprehend without completely overhauling it. I've ended up doing that in #27788.

📝 achow101 converted_to_draft 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
...

💬 theStack commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1210987152)
Regarding the goal of simple implementation, is there a strong need to keep the `FE.is_square` method? Right now this is the only call-site, which I think could be replaced by
```suggestion
return (FE(x)**3 + 7).sqrt() is not None
```
I assume trying to actually square is way slower than computing the Jacobi symbol, but since `is_valid_x` doesn't appear to be in a critical code-path, that's probably fine? At least for `feature_taproot.py` I didn't see a decrease in performance.

💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569405783)
reACK 2808c33bae95a0d2516a6e9a550032af142a04de

(I wouldn't expect bitcoin-cli to convert the types)

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031592)
Moved the `Assume` to `LoadRecords`.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031688)
No, added the exception handling back.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031768)
Added it back.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211031914)
Don't remember, undone. Probably a bad rebase?

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032001)
Done as suggested.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032031)
Done as suggested.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032083)
Done as suggested.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032129)
Done as suggested.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032152)
Done as suggested.

💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032186)
Done as suggested.

📝 achow101 opened a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790)
Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917

This PR adds a database cursor that gives a view over all of the records beginning with the same prefix.