👍 MarcoFalke approved a pull request: "test: (refactor) Use datadir from options in chainstatemanager test"
(https://github.com/bitcoin/bitcoin/pull/27876#pullrequestreview-1477197363)
lgtm ACK d54819d74e04e6105c1f0362755f5bcfa845eefd

💬 MarcoFalke commented on pull request "test: (refactor) Use datadir from options in chainstatemanager test":
(https://github.com/bitcoin/bitcoin/pull/27876#discussion_r1228144723)
review note, this creates a copy and is not a reference, thus safe

💬 glozow commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1589349516)
Concept ACK

💬 glozow commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1589359272)
https://cirrus-ci.com/task/6053265947754496 ? 😢

💬 dimitaracev commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#discussion_r1228179276)
Did you mean `default is to suppress`?

💬 dergoegge commented on pull request "build: make sure we can overwrite config.{guess,sub} before doing so":
(https://github.com/bitcoin/bitcoin/pull/27875#issuecomment-1589385892)
Concept ACK

I've been having this issue on my machine, will test.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1228189310)
The actual error ends up being generic:

```
Error: A fatal internal error occurred, see debug.log for details
```

and actually before #27708 was merged yesterday, this code was even more messy!


The problem I'm having now is with CI: on only a few of the test platforms bitcoind does not crash as expected. This means that either the python line `os.chmod(...)` is not working to change the permissions of the `.blk` file OR for whatever reason the OS is happy to `open()` files with `rb
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1211719573)
nit: spurious?

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1211693431)
nit (here and below): redundant to check for emptiness before asserting the size is 1

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1211675298)
Looks spurious?

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226847502)
Let's also cover Miniscript (along with `wsh`)?
```diff
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 3ea8d63ab3..adbaf48e77 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -710,6 +710,31 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
{{6, 0}, {6, 1}, {6, 2}, {3, 0, 0}, {3, 0, 1}, {3, 0, 2}, {0, 0, 5, 0}, {0, 0, 5, 1}, {0, 0, 5, 2}},
}
);
+ CheckMultipath("wsh(or_d(pk([2557c640/48h/1h/0
...

🤔 darosior reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-1444202031)
ACK on most of the core logic. Just found a small mistake in the parsing of the multipath step in the derivation path. I also have a couple questions on the modifications to the RPC interface. The rest is nits, feel free to ignore them. Nice set of unit tests!

One almost-nit but important for posterity IMO is that all your commit messages mention the previous iteration of the BIP that only allowed for 2 paths (change and receive) and should be adapted to mention "multiple" paths to avoid conf
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226858466)
The error is specific to 2-paths multipaths. But it's removed in a following commit anyways.

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226766049)
Maybe worth checking across the branches of a Miniscript too?

```diff
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 3ea8d63ab3..e1864652d8 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -716,6 +731,7 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/<6;7;8;9>/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpg
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226687559)
nit: s/subscripts/derivation paths/

I guess "subscripts" is a copy pasta from the `tr()` error?

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880)
This could be a `CHECKNONFATAL` that it's never empty, as any sane Miniscript must contain at least one key check.

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1227924512)
nit: the docstring could be updated with the new meaning and parameter.

💬 0xB10C commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1589403896)
ACK 3ef756a5b558a1dd2fcb93bc0d4237707aa04f3f. I've focused my testing and code review on the tracepoint related changes. The docs, the test, and the mempool_monitor.py demo script are updated. I ran the `interface_usdt_mempool.py` test and the `mempool_monitor.py` script. The `mempool_monitor.py` output looks correct.

>> Something that just occurred to me: do/should we consider tracepoints to be a stable api?
>
> No, we shouldn't. I think that classes like CTxMemPoolEntry and their public a
...

🤔 darosior reviewed a pull request: "wallet: Give deprecation warning when loading a legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/27869#pullrequestreview-1477293036)
Concept ACK. How about also logging for wallets that are loaded on startup?

💬 darosior commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#discussion_r1228204662)
Maybe hint at how to migrate?

💬 achow101 commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589417552)
> so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?

I get a large number of warnings from boost.