💬 fjahr commented on pull request "doc: update NeedsRedownload() and nStatus comment":
(https://github.com/bitcoin/bitcoin/pull/29624#issuecomment-2355678474)
ACK af9f9878934f88036423021c70ef523b6c9e1c90
(https://github.com/bitcoin/bitcoin/pull/29624#issuecomment-2355678474)
ACK af9f9878934f88036423021c70ef523b6c9e1c90
💬 hebasto commented on pull request "cmake: Switch to crc32c upstream build system":
(https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-2355688236)
> This PR is also changing the flags used for code generation.
Yes. The `crc32c` upstream build system modifies flags in many places, for [example](https://github.com/bitcoin-core/crc32c-subtree/blob/efb8ea04e4a5b6a18dc4bc1908fd1cb2dcefb585/CMakeLists.txt#L58-L64):
```cmake
# Disable C++ exceptions.
string(REGEX REPLACE "-fexceptions" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions")
# Disable RTTI.
string(REGEX REPLACE "-fr
...
(https://github.com/bitcoin/bitcoin/pull/30905#issuecomment-2355688236)
> This PR is also changing the flags used for code generation.
Yes. The `crc32c` upstream build system modifies flags in many places, for [example](https://github.com/bitcoin-core/crc32c-subtree/blob/efb8ea04e4a5b6a18dc4bc1908fd1cb2dcefb585/CMakeLists.txt#L58-L64):
```cmake
# Disable C++ exceptions.
string(REGEX REPLACE "-fexceptions" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions")
# Disable RTTI.
string(REGEX REPLACE "-fr
...
📝 MichaelXCity opened a pull request: "Bitcoil"
(https://github.com/bitcoin/bitcoin/pull/30917)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoil Core user experience or Bitcoil Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30917)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoil Core user experience or Bitcoil Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ hebasto closed a pull request: "Bitcoil"
(https://github.com/bitcoin/bitcoin/pull/30917)
(https://github.com/bitcoin/bitcoin/pull/30917)
📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/30917)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoil Core user experience or Bitcoil Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30917)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoil Core user experience or Bitcoil Core developer experience
significantly:
* Any test improvements or new tests that improv
...
👍 willcl-ark approved a pull request: "ci: Print inner env, Make ccache config more flexible"
(https://github.com/bitcoin/bitcoin/pull/30869#pullrequestreview-2309737230)
ACK fa99e4521b6fc0e7f6636d40bc0d6a7325227374
Making ccache mor configurable in CI seems like a reasonable change overall. Hopefully in the future this could help us if we moved to some more powerful CI runners (with bigger ccache!)
(https://github.com/bitcoin/bitcoin/pull/30869#pullrequestreview-2309737230)
ACK fa99e4521b6fc0e7f6636d40bc0d6a7325227374
Making ccache mor configurable in CI seems like a reasonable change overall. Hopefully in the future this could help us if we moved to some more powerful CI runners (with bigger ccache!)
💬 willcl-ark commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1763232646)
This does work, but seems to come out a little garbled in the raw logs on CI itself. It didn't happen for me locally and it does not appear to be fault of the script itself, so I guess just GitHub UI processing stdout slowly or something, e.g.
```log
2024-09-16T15:22:03.4198819Z + echo '=== END env ==='
2024-09-16T15:22:03.4199084Z + tee /dev/fd/63
2024-09-16T15:22:03.4199339Z ++ patch -p1
2024-09-16T15:22:03.4199590Z + '[' false = true ']'
2024-09-16T15:22:03.4199872Z + '[' true = true
...
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1763232646)
This does work, but seems to come out a little garbled in the raw logs on CI itself. It didn't happen for me locally and it does not appear to be fault of the script itself, so I guess just GitHub UI processing stdout slowly or something, e.g.
```log
2024-09-16T15:22:03.4198819Z + echo '=== END env ==='
2024-09-16T15:22:03.4199084Z + tee /dev/fd/63
2024-09-16T15:22:03.4199339Z ++ patch -p1
2024-09-16T15:22:03.4199590Z + '[' false = true ']'
2024-09-16T15:22:03.4199872Z + '[' true = true
...
👍 hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2309767506)
re-ACK b9aded42252969cc81acf4122ca5e2d1e00ecf90
Checked changes since last ACK with `git range-diff master 69bf58d b9aded4`.
- Adjusted commit message to not mention indirect documentation generation.
- Incorporated feedback around .dat-file paths being for legacy wallets.
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2309767506)
re-ACK b9aded42252969cc81acf4122ca5e2d1e00ecf90
Checked changes since last ACK with `git range-diff master 69bf58d b9aded4`.
- Adjusted commit message to not mention indirect documentation generation.
- Incorporated feedback around .dat-file paths being for legacy wallets.
💬 fjahr commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2355828054)
> actually, that didn't work because NotifyHeaderTip can't be called when cs_main is held. Reverting this, I'll think about this a bit more.
Is this ready for review or should we wait until you have addressed this @mzumsande ?
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2355828054)
> actually, that didn't work because NotifyHeaderTip can't be called when cs_main is held. Reverting this, I'll think about this a bit more.
Is this ready for review or should we wait until you have addressed this @mzumsande ?
💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2355833959)
> > So I think the best way forward would be to have all workers of the same type on the same machine. (I am still running some performance tests to get an estimate if this make sense at all)
Is this so that re-runs (of the same PR) always hit the cache? If that's it I'm not _totally_ sure it's worth it. My intuition says that if we had multiple servers, even if a re-run took place on a different machine, we'd likely get a *pretty decent* cache hit from another run or a previous master build
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2355833959)
> > So I think the best way forward would be to have all workers of the same type on the same machine. (I am still running some performance tests to get an estimate if this make sense at all)
Is this so that re-runs (of the same PR) always hit the cache? If that's it I'm not _totally_ sure it's worth it. My intuition says that if we had multiple servers, even if a re-run took place on a different machine, we'd likely get a *pretty decent* cache hit from another run or a previous master build
...
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2355865265)
@marcofleon Example checks out, it's pretty simple:
1) txA is called with `txdownload_impl.MempoolRejectedTx` and `TX_RECONSIDERABLE`
2) txA is called with `txdownload_impl.MempoolRejectedTx` and `TX_NOT_STANDARD` or any failure that results in entry to reject filter
3) `ReceivedTx` is called with txA, child is found in orphanage, package returned even though txA is in both filters.
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2355865265)
@marcofleon Example checks out, it's pretty simple:
1) txA is called with `txdownload_impl.MempoolRejectedTx` and `TX_RECONSIDERABLE`
2) txA is called with `txdownload_impl.MempoolRejectedTx` and `TX_NOT_STANDARD` or any failure that results in entry to reject filter
3) `ReceivedTx` is called with txA, child is found in orphanage, package returned even though txA is in both filters.
💬 theuni commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2355883704)
> Concept ACK.
>
> [a39a915](https://github.com/bitcoin/bitcoin/commit/a39a915c226775396f6505efe77a4aba4d0ae3ad) Did you consider setting `CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY` globally?
>
Yes, but I didn't feel as comfortable setting this one globally due to our (for ex) phony deploy targets. I figured we may make some changes in the future that could introduce hard-to-track-down bugs. It's clear to see that the header generation is self-contained, and afaik that's the only plac
...
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2355883704)
> Concept ACK.
>
> [a39a915](https://github.com/bitcoin/bitcoin/commit/a39a915c226775396f6505efe77a4aba4d0ae3ad) Did you consider setting `CMAKE_ADD_CUSTOM_COMMAND_DEPENDS_EXPLICIT_ONLY` globally?
>
Yes, but I didn't feel as comfortable setting this one globally due to our (for ex) phony deploy targets. I figured we may make some changes in the future that could introduce hard-to-track-down bugs. It's clear to see that the header generation is self-contained, and afaik that's the only plac
...
👍 willcl-ark approved a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2309846724)
tACK 2a581144f28bad44de40122864f2f7b9fc5000de
I didn't review the code in too much detail, but I like hebasto also saw a good speedup on Ubuntu 23.10 x86_64 from 14 seconds down to half a second for block413567.raw with `cmake` 3.30:
```log
will@ubuntu in ~/src/bitcoin on master [$?⇕] via △ v3.30.3 : 🐍 (bitcoin) took 12s
$ rm -i -Rf build
will@ubuntu in ~/src/bitcoin on master [$?⇕] via △ v3.30.3 : 🐍 (bitcoin)
$ time cmake -DRAW_SOURCE_PATH=src/bench/data/block413567.raw -DHEAD
...
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2309846724)
tACK 2a581144f28bad44de40122864f2f7b9fc5000de
I didn't review the code in too much detail, but I like hebasto also saw a good speedup on Ubuntu 23.10 x86_64 from 14 seconds down to half a second for block413567.raw with `cmake` 3.30:
```log
will@ubuntu in ~/src/bitcoin on master [$?⇕] via △ v3.30.3 : 🐍 (bitcoin) took 12s
$ rm -i -Rf build
will@ubuntu in ~/src/bitcoin on master [$?⇕] via △ v3.30.3 : 🐍 (bitcoin)
$ time cmake -DRAW_SOURCE_PATH=src/bench/data/block413567.raw -DHEAD
...
💬 TheCharlatan commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763300138)
Ah right, I don't think this a blocker for this PR anyway, just something that came to my mind.
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763300138)
Ah right, I don't think this a blocker for this PR anyway, just something that came to my mind.
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2355903145)
> Should I modify this PR now to be on top of yours, including only the second test? Or would it be better to wait until your PR gets merged and then modify mine?
I would recommend you wait a bit to see what the feedback on that PR is.
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2355903145)
> Should I modify this PR now to be on top of yours, including only the second test? Or would it be better to wait until your PR gets merged and then modify mine?
I would recommend you wait a bit to see what the feedback on that PR is.
🚀 fanquake merged a pull request: "ci: Use macos-14 GHA image (x86_64-apple-darwin22.6.0 -> arm64-apple-darwin23.6.0)"
(https://github.com/bitcoin/bitcoin/pull/30913)
(https://github.com/bitcoin/bitcoin/pull/30913)
🚀 fanquake merged a pull request: "Remove Autotools packages from CI (and depends doc)"
(https://github.com/bitcoin/bitcoin/pull/30902)
(https://github.com/bitcoin/bitcoin/pull/30902)
🚀 fanquake merged a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888)
(https://github.com/bitcoin/bitcoin/pull/30888)
🚀 fanquake merged a pull request: "doc: update NeedsRedownload() and nStatus comment"
(https://github.com/bitcoin/bitcoin/pull/29624)
(https://github.com/bitcoin/bitcoin/pull/29624)
👍 hodlinator approved a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2309847632)
ACK fabd78e2e30e557e04739e29c9c221ad47245df1
Concept: Great to have compile time format string error checking on the common logging functions!
Tested changing this in *logging.h*:
```diff
- log_msg = tfm::format(fmt, params...);
+ log_msg = tfm::format("%s %s\n", 1.1f);
```
and ran with expected error output.
Agree with ryanofsky that it would be good to document rough compile time impact if any.
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2309847632)
ACK fabd78e2e30e557e04739e29c9c221ad47245df1
Concept: Great to have compile time format string error checking on the common logging functions!
Tested changing this in *logging.h*:
```diff
- log_msg = tfm::format(fmt, params...);
+ log_msg = tfm::format("%s %s\n", 1.1f);
```
and ran with expected error output.
Agree with ryanofsky that it would be good to document rough compile time impact if any.