💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2845582142)
> While reviewing this I hit some weird behavior where `bitcoin-node` doesn't fully shutdown.

Thanks for testing! These are all known issues which should be fixed by https://github.com/bitcoin/bitcoin/pull/32345

💬 mustafacryptolife commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#discussion_r2070707481)
[[[****]()]()]()

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070717157)
Thanks, I used your suggestion.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070718791)
Right, it checks whether the expected result and the selected input set match, but the message here is printed in the case of a failure!

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2845620399)
> I was referring to something like the code below (which encapsulates the latest commit changes), but it was just a suggestion. The current code looks good to me too.

I see, thanks. I guess it could be nice to be able to run the test suite in smaller portions especially if some of the tests took a long time, but it overall runs extremely quickly, so I’m not sure it is necessary to further structure the tests at this time.

I intend to work on porting the other algorithms’ tests in the same
...

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2845620960)
Should be ready to review, again.

💬 instagibbs commented on issue "p2p: lingering entries in `mapBlockSource`":
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-2845639973)
If we don't think a whole fix is in the cards soon, I'm partial to adding a test mechanically checking the weird behavior.

👍 pinheadmz approved a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2810346702)
ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa

Built and ran tests on arm64/macos. Played with the feature locally. Reviewed each commit, some questions below.

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

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

ACK 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgT0ykACgkQ5+KYS2KJ
yTogNQ/+NxdemF18NTDuoW6W7fLeDbdbJCbe/9fSse0fhFi0rtTkFpJdXKt+p86T
gndlLfTNitJ58BUul5WMbJ
...

💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845)
ea98a42640b9ff77a462e55887025ddd1da54727

Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`

💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693)
ea98a42640b9ff77a462e55887025ddd1da54727

I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.

You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.

I guess you need to parse the request to get the method before checking the whitelist, but I just wonder i
...

💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369)
ea98a42640b9ff77a462e55887025ddd1da54727

Might want to indicate `status` is an "out" param?

💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092)
113b46d31075c66cecbe4482aed362d50cdec28c

I presume you can ignore `rpcwait` because the socket won't exist until the server is running?

What about the try/catch? There's no "connection failed" case like there is for HTTP?

💬 pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905)
9ff5bc75659e0314f42c1477fc0c37ac0ad132fa

Hm for some reason I thought it could be as low as 92:

https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/test/functional/feature_proxy.py#L66-L67

💬 davidgumberg commented on issue "test: functional test failures under -DENABLE_WALLET=OFF":
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2845668503)
I think #31250 genuinely fixed this, https://github.com/bitcoin/bitcoin/pull/31250/commits/c847dee1488a294c9a9632a00ba1134b21e41947, changed the logic for whether or not tests are run in all the crash cases exhibited here:

```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index c7f0cc5e43..ee79a27392 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -567,7 +565,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert
...

💬 monlovesmango commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070755965)
Ok I see! I understand now. Sorry was running with `--log_level=all` and was misinterpreting these as success messages. Please disregard..

🤔 1440000bytes reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2810654144)
utACK https://github.com/bitcoin/bitcoin/pull/29365/commits/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1

Isn't the commit message too long? You could even have multiple commits in this pull request IMO.

💬 hebasto commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2845818802)
> > During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
>
> Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them. This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there's nothing documented to say they should be excluded.

https://github.com/arun11299/cpp-sub
...

💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2845902580)
> utACK [41728b5](https://github.com/bitcoin/bitcoin/commit/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1)
>
> Isn't the commit message too long? You could even have multiple commits in this pull request IMO.

It is possible to split the commit (and the commit message) into multiple parts:

- add function `ParseWrappedSignetChallenge` and its unit test (files `src/chainparams.{h,cpp} src/test/chainparams_tests.cpp src/test/CMakeLists.txt`)
- add function `ParseSignetParams` and its unit t
...

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070888721)
Thanks for taking such a thorough look!

💬 davidgumberg commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2070888806)
In commit ef83e9569752b4273e6aed3124642dc057cab540 (shutdown: Tear down mempool after chainman)

Feel free to disregard as out of scope, but while addressing this, it might be nice to make reintroducing this dangling pointer impossible by making the various mempool pointers `shared_ptr`, e.g. https://github.com/davidgumberg/bitcoin/commit/75c839b8d77c207a0946294672b703d9f8eecbe1