💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548332581)
fwiw I'm ok getting this in for backport. This is most cruft at the RPC layer that can be reverted on the next PR.

💬 codexnakamoto commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1548338939)
Concept ACK - this is logical and seems uncontentious.

👍 pinheadmz approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1426880326)
ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef

Code review and run bitcoind and -qt with various conf file configurations to see errors and warnings. A few style nits below, can be ignored (especially the release note since that will get reviewed again upon actual release)

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 0319de5cbedd1a8f8766cfec61151c58b3fb27ef
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJ
...

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194133266)
nit: indent should be +2 more spaces

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194151784)
I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:


> Data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin/whoa" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf" from data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin" is being used instead.

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194040447)
```suggestion
- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that will be ignored, which can happen when a datadir= line is used in a bitcoin.conf file. The error message is just a diagnostic intended to prevent accidental misconfiguration, and it can be disabled to restore the previous behavior of using the datadir while ignoring the bitcoin.conf contained in it.
```

💬 furszy commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548438693)
> > So, probably the `importdescriptors` RPC command call was done with `timestamp=now` and no rescan was performed.
>
> I confirm that there was the `importdescriptors` RPC command call done with `timestamp=now` and no rescan was performed.

Based on that, maybe close this issue?

I think that adding the "Do you want to start rescanning Bitcoin blockchain in order to update the balance?" inside the sending process is an overkill (it will affect other scenarios and not only this one), lea
...

📝 brunoerg opened a pull request: "docs: fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/27664)
Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier)

💬 pinheadmz commented on pull request "test: add unit test coverage for Python ECDSA implementation":
(https://github.com/bitcoin/bitcoin/pull/27653#discussion_r1194287587)
IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that `generate_privkey()` is always valid but certain edge cases are not (`0`, `SECP256K1_ORDER`, and `2**256 - 1`)

🤔 pinheadmz reviewed a pull request: "test: add unit test coverage for Python ECDSA implementation"
(https://github.com/bitcoin/bitcoin/pull/27653#pullrequestreview-1427256339)
concept ACK

I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the `key_tests.cpp` unit test, assuring that both implementations handle Schnorr (at least those cases) identically.

💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548505174)
> My understanding is that `REPLACED` would also need to be provided to peers: [#27625 (comment)](https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255). Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool (and not yet in a block, because it may be in-flight)?

I think it's better that we not relay transactions that are `REPLACED` -- note that we
...

💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194318233)
`m_node_address` is a string, which is part of AddedNodeParams, which has just the unvalidated user input.

`resolvedAddress` is a `CService` built from `m_node_address`, which is generated in `GetAddedNodeInfo` and used in `ThreadOpenConnection` / rpc code.

💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548517395)
There is a bugfix in here, because I think it fixes the submitpackage acceptance policy on regtest by restricting it to a safe subset of topologies? Users may use regtest now to prepare for usage of the rpc on mainnet later.

📝 achow101 opened a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665)
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.

I thought this was included in #26715, maybe it got lost in a rebase.

👍 furszy approved a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427332880)
utACK 0282b212

📝 achow101 opened a pull request: "wallet, bench: Move commonly used functions to their own file and fix a bug"
(https://github.com/bitcoin/bitcoin/pull/27666)
I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.

The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.

The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues curr
...

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1548545937)
First 2 commits split into #27666. Marking as draft for now.

📝 achow101 converted_to_draft a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008)
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.

As these functions are based on doing `IsMine` for each `Script
...

💬 ismaelsadeeq commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1194346683)
Concept ACK
Nit: As mentioned in review club, it would be nice to use a more descriptive variable name for the fr variable, considering your change is within the `transformNamedArguments` method. like find_result or a similar alternative.
```suggestion
auto find_result = argsIn.end();
```

💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194346880)
Yes, thanks, that's a bit simpler and should be equivalent, so I took your suggestion!