💬 S3RK commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-1837481952)
Concept ACK

💬 S3RK commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1413081344)
`Replace` is not correct, because the old keys can still have coins on them.

We should recommend:
a) making a new backup, bot not removing/replacing the old one just yet
b) optional. In case the user believes the old keys could be compromised then direct them to sweep the old keys, e.g. with `sendall` RPC

🤔 furszy reviewed a pull request: "test: allow BITCOIN_TEST_PATH to specify working dir"
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1761271269)
Instead of introducing the new environment variable, could enable the existing `-datadir=<path>` arg capability in this way https://github.com/furszy/bitcoin-core/commit/135147aaa69f40aa59109f1b6f1760a125ed36e0 for the unit test framework.

Example of its usage:
```bash
/test_bitcoin --run_test=txindex_tests -- -datadir="/path/to/dir"
```

I think it is simpler and easier to remind as it does not change the way people/we use any core binary.
Moreover, expanding any other binary with this
...

💬 furszy commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1413130879)
> UPD: After thinking more, I understood it depends on how the users performs the back up. If they back up the whole wallet file - it's safe. But just backing up the new HD seed is not safe. So, it seems recommending replacing backup is still slightly dangerous.

Well, core currently lacks a mechanism for retrieving the HD seed. It refrains from outputting the seed post wallet creation and advises the user to back up the entire wallet file instead.
The same procedure can be seen after encrypt
...

💬 furszy commented on pull request "test: Fix test by checking the actual exception instance":
(https://github.com/bitcoin/bitcoin/pull/28989#discussion_r1413137963)
Could use `HasReason()`.
```c++
BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON"));
```

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1413138828)
This flag is only used to connect to limited peers or not. In that case, why not remove the indirection about syncing and just call it `allow_limited_peers`?

✅ furszy closed a pull request: "wallet: remove unused DummySignTx and CKeyPool from GetReservedDestination"
(https://github.com/bitcoin/bitcoin/pull/25881)

💬 furszy commented on pull request "wallet: remove unused DummySignTx and CKeyPool from GetReservedDestination":
(https://github.com/bitcoin/bitcoin/pull/25881#issuecomment-1837520765)
Closing it for now. Have other PRs with more priority than this one.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1413143266)
> This flag is only used to connect to limited peers or not. In that case, why not remove the indirection about syncing and just call it `allow_limited_peers`?

Because of what I wrote above, this flag can (and most likely will) be used in other places as well, fulfilling two goals: (1) minimizing `cs_main` locks, which improves the overall node response, and (2) minimizing the usage of the chainstate IBD flag, which lacks back-and-forth functionality

💬 hebasto commented on pull request "test: Fix test by checking the actual exception instance":
(https://github.com/bitcoin/bitcoin/pull/28989#issuecomment-1837526938)
Updated 24d978a36d1b405f968161a9d046beffffc0b249 -> 55e3dc3e03510e97caba1547a82e3e022b0bbd42 ([pr28989.01](https://github.com/hebasto/bitcoin/commits/pr28989.01) -> [pr28989.02](https://github.com/hebasto/bitcoin/commits/pr28989.02), [diff](https://github.com/hebasto/bitcoin/compare/pr28989.01..pr28989.02)):

- addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/28989#discussion_r1413137963)

💬 hebasto commented on pull request "test: Fix test by checking the actual exception instance":
(https://github.com/bitcoin/bitcoin/pull/28989#discussion_r1413145308)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/28989#issuecomment-1837526938).

🤔 furszy reviewed a pull request: "test: Fix test by checking the actual exception instance"
(https://github.com/bitcoin/bitcoin/pull/28989#pullrequestreview-1761301944)
Non-Windows code ACK 55e3dc3e

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1413149420)
Right, then I think `m_is_close_to_tip` would make the most sense, since it's set initially after calling `IsBlockCloseToTip`.

👍 andrewtoth approved a pull request: "cli: improve error message on multiwallet and add validation to cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-1761314443)
ACK 755320f75f2141909e84b62f420462c1c5b193e6

💬 andrewtoth commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1413160578)
nit: `3. unrecognized cli-commands or other arguments starting with a slash`

💬 andrewtoth commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1413159532)
nit:
```
* @returns true if cli_command is included in the set of the exclusively_commands.
```

💬 andrewtoth commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1413159552)
nit: could be `static`.

💬 andrewtoth commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1413159420)
nit: This parameter is not a reference. It is being passed by value. This comment could be more descriptive eg:
```
@param[in] cli_command The string to check if it is a cli-command that needs to run exclusively.
```

💬 andrewtoth commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1413160472)
nit: `2. multiple bitcoin-cli commands that need to run exclusively`

👍 andrewtoth approved a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1761328425)
ACK a0687ffd568b0984a8b1e51fd0061f87f25e95d7