π achow101 merged a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611)
(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)
(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!
(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?
(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.
(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
...
(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.
(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
(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)
[[[****]()]()]()
(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.
(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!
(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
...
(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.
(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.
(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
...
(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`
(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
...
(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?
(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?
(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
(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