👋 TheCharlatan's pull request is ready for review: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960)

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1837267077)
Undrafting this now. I tested this approach extensively against my proof of concept [C FFI](https://github.com/TheCharlatan/bitcoin/pull/7) and proof of concept [Rust library](https://github.com/TheCharlatan/rust-bitcoinkernel), and think it's not that bad an interface to have. Not sure why the msvc build is failing (only seems to be the Qt build?).

💬 hebasto commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1837269861)
> Not sure why the msvc build is failing (only seems to be the Qt build?).

cc @sipsorcery

💬 aureleoules commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1837290207)
The benchmark `WalletCreateTxUsePresetInputsAndCoinSelection` seems to segfault occasionally on this pull (increasing min-time makes it crash consistently)
```bash
$ valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==131665== Memcheck, a memory error detector
==131665== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==131665== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==131665== Command: ./src
...

👍 pablomartin4btc approved a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1760994448)
re ACK 30983e4e30a136ce7d2efc3639f0c17ed89ea38b

💬 theStack commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1837299151)
> I switched to the `Popen` constructor that accepts `const std::string&` instead of `std::vector<std::string>` to address quoting issues on Windows.
>
> @theStack Since you initially chose to use a vector, do you have any objections to this approach?

No objections at all. I would have expected that passing the arguments as a single string is _more_ prone to quoting issues than if it's done in a list, where quoting shouldn't be needed at all, but if it works, all the better :) (Maybe that
...

💬 Sapibeltv commented on issue "Libbitcoinkernel Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-1837312263)
We are sapibel families and Seexy Nadege Entertainment 2vtv and SAPIBELTVradio is the most important tools to use the internet marketing industry provider https://github.com/bitcoin/bitcoin/assets/107956561/38841f2f-3194-4f66-aa76-9eeb96988216

💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1837430222)
> > I switched to the `Popen` constructor that accepts `const std::string&` instead of `std::vector<std::string>` to address quoting issues on Windows.
> > @theStack Since you initially chose to use a vector, do you have any objections to this approach?
>
> No objections at all. I would have expected that passing the arguments as a single string is _more_ prone to quoting issues than if it's done in a list, where quoting shouldn't be needed at all, but if it works, all the better :) (Maybe t
...

💬 S3RK commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1837467046)
Concept ACK.

The PR description should say that we don't use hardcoded fee rate value, but rather multiple of long-term-fee-rate. LTFR is an existing user configurable parameter. It's also a good fit for the purpose of determining when to be more "aggressive" with coin selection, because it captures forward looking fee market expectations.

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