Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸš€ achow101 merged a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611)
βœ… fanquake closed a pull request: "chore(ci): bump docker/build-push-action to v6.16.0"
(https://github.com/bitcoin/bitcoin/pull/32398)
πŸ’¬ andrewtoth commented on issue ""Rolling forward" at startup can take a long time, and is not interruptible (after unclean shutdown)":
(https://github.com/bitcoin/bitcoin/issues/11600#issuecomment-2845541462)
Woohoo!
πŸ’¬ strmfos commented on pull request "chore(ci): bump docker/build-push-action to v6.16.0":
(https://github.com/bitcoin/bitcoin/pull/32398#issuecomment-2845545697)
Hi! [fanquake](https://github.com/fanquake)
what's wrong?
πŸ’¬ hebasto commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2845558709)
> from a directory with spaces in its path, CMake issues the following warning:
>
> ```
> Warning: Paths with embedded space may be handled incorrectly by configure
> ```

This is a vcpkg upstream issue. Another one has already been documented:https://github.com/bitcoin/bitcoin/blame/5b8046a6e893b7fad5a93631e6d1e70db31878af/doc/build-windows-msvc.md#L62-L72

This is not specific to CMake in general, nor to the Bitcoin Core build system.
πŸ’¬ pinheadmz commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2845567909)
While reviewing this I hit some weird behavior where `bitcoin-node` doesn't fully shutdown.

To reproduce:

In terminal 1: `build/bin/bitcoin-node -regtest -ipcbind=unix -debug=rpc -debug=http -debug=ipc`

Terminal 2: `build/bin/bitcoin-cli -regtest waitforblock 0000000000079f8ef3d2c688c244eb7a4570b24c9ed7b4a8c619eb02596f8862`
(this will wait forever)

go back to terminal 1 and ctrl-c. I get `Shutdown: done` in the output but the process does not quit.

Then funny stuff like this, in
...
πŸ’¬ Brotcrunsher commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2845580066)
@hebasto Thanks for your feedback! I fixed the wording in the commit message to mention vcpkg instead of CMake. I agree that this isn't Bitcoin Cores fault, but IMO it would still be "nice" for new devs to have a warning in place. Might save them a minor headache.
πŸ’¬ 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