💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1837238921)
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?

---

Here are the Windows functional tests -- https://github.com/hebasto/bitcoin/actions/runs/7072075125/job/19250502576

💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1412875672)
You are right, I pushed a fix.

👋 hebasto's pull request is ready for review: "POC: Replace Boost.Process with cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/28981)

💬 hebasto commented on pull request "POC: Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1837257182)
CI turned green. Undrafted.

cc @achow101 @Sjors

⚠️ Antwan52 opened an issue: "?"
(https://github.com/bitcoin/bitcoin/issues/28988)
0xb0e8EF343c70170FBd50f42917FE9Bd21300a5ff

✅ fanquake closed an issue: "?"
(https://github.com/bitcoin/bitcoin/issues/28988)

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

💬 brunoerg commented on pull request "test: add coverage for bech32m in `wallet_keypool_topup`":
(https://github.com/bitcoin/bitcoin/pull/28928#issuecomment-1837259374)
Good idea, @furszy.

master: 1.78s user 0.57s system 28% cpu 8.157 total

this PR: 0.76s user 0.20s system 35% cpu 2.668 total

📝 hebasto opened a pull request: "test: Fix test by checking the actual exception instance"
(https://github.com/bitcoin/bitcoin/pull/28989)
The `system_tests/run_command` test is broken because it passes even with the diff as follows:
```diff
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -90,7 +90,7 @@ BOOST_AUTO_TEST_CASE(run_command)
});
}
{
- BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON
+ BOOST_REQUIRE_THROW(RunCommandParseJSON("invalid_command \"{\""), std::runtime_error); // Unable to parse JSON
}
//
...

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