Bitcoin Core Github
43 subscribers
122K links
Download Telegram
💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2490590565)
> Sounds like a TODO? Why not "Fixes https://github.com/bitcoin/bitcoin/issues/31334"?

Sure, changed to fixes. However it is a TODO in the sense that we shouldn't need to do any of this for a working CI, and ideally, these changes should be removed at some point, as they are all workaround for other problems.
💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2490592436)
I also split out the renames in 55f2bb7a96806d33e475122f4d8f54b117838afe. They were an accidental find & replace, but useful to be consistent with the new `BlockCreateOptions` field.
💬 maflcko commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490598011)
> As far as I'm aware, Clang based coverage builds are working outside of oss-fuzz (cc @dergoegge), so it's not clear why this would be the right solution. If it's an oss-fuzz only problem, shouldn't the fix happen there?

This also affects GCC coverage, see https://cirrus-ci.com/github/maflcko/b-c-cov/ci, so the fix here is needed and likely correct (modulo my question/nit about the guix build).
💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851686045)
Nit: Is there one `coinbaseaux` too many here?
💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851693999)
In commit b35b23d823ac0fc43e4d137525167a7788ad672a:

Nit: I think we just directly call the new function here?
💬 fanquake commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490602573)
Sure, but it'd be good to get a better explanation as to how Clang builds in oss-fuzz are broken, if Clang builds outside of oss-fuzz are working.
💬 maflcko commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851712070)
Looks like https://github.com/bitcoin/bitcoin/pull/31337/checks passed CI recently, so maybe this was just a temporary issue?
💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851712986)
This change was premature since the interface is changed in a later commit. It also doesn't do anything, so I'm dropping it.
💬 dergoegge commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)
Here is a recent example of clang coverage working for me: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin/harnesses/rpc/coverage_report/coverage/workdir/bitcoin/index.html

I'm slightly confused why oss-fuzz broke while my setup keeps working. Afaict I'm using the same flags and `llvm-*` tools as them...
💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2490632989)
I re-combined d31e5f9d9986ce1e80b014a1585ab0e944ebcc86 and d1972d751fa882960375050bfa7fc6f4043e0be4 so that the interface change is done along with the change to CreateNewBlock. This seems more clear than having them separate.
👍 dergoegge approved a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333#pullrequestreview-2450864181)
utACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49
💬 Sjors commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851743969)
The reason I did it differently was that `interfaces.cpp` the pattern `blah && blah.value()` makes it clear the first value is a pointer rather than a boolean. That's not clear here. That said, I don't think anyone would be confused either.
💬 Sjors commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851747612)
That's probably worth clarifying in a comment...
💬 Sjors commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851750132)
I didn't think about it that deeply, but yes.
💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851750952)
Dropped the workaround, and it seems like using `pkgconf` is still broken, so we'd need to retain the workaround. We could also just update only the docs, and change the CI later if it starts breaking again.
💬 maflcko commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851765882)
I'd presume you could only update the CI after GHA has updated their image?
💬 hebasto commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490733585)
> I'm slightly confused why oss-fuzz broke while my setup keeps working. Afaict I'm using the same flags and `llvm-*` tools as them...

They [use](https://github.com/google/oss-fuzz/blob/51922e43fa80ae858fcd12aa84be3c0c75ed7b0d/infra/base-images/base-runner/coverage#L54-L55) the `-path-equivalence=...` option:
```bash
# Use path mapping, as $SRC directory from the builder is copied into $OUT/$SRC.
PATH_EQUIVALENCE_ARGS="-path-equivalence=/,$OUT"
```
💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851793270)
Probably. Added a note for that, dropping the package change, but now the build is failing again, with the same as #31334.
💬 hebasto commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851796441)
Or we can `brew update`. However, it might be time consuming.
💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1851804865)
Still worried that this will backfire, but will investigate more and maybe spawn a new PR.