Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👍 instagibbs approved a pull request: "feefrac: avoid explicitly computing diagram; compare based on chunks"
(https://github.com/bitcoin/bitcoin/pull/29757#pullrequestreview-1976038089)
ACK 4bf7e1a005853cb57ae3a051bcdfc5c149b1f1db

didn't run fuzz tests
💬 instagibbs commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1549280078)
```suggestion
// The total fee & size of the new diagram minus replaced fee & size should be the total fee & size of the old
```
💬 instagibbs commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1549281309)
```suggestion
* The sum of the FeeFracs in either of the chunks' data sets cannot overflow (sum fees < 2^63,
```
💬 instagibbs commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1549266433)
might want to make it clearer if this is descriptive or prescriptive
💬 fanquake commented on pull request "ci: Print tsan errors to stderr":
(https://github.com/bitcoin/bitcoin/pull/29740#issuecomment-2033984929)
Pulled this into 27.x in #29780.
💬 0xB10C commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2033993210)
> > I'm missing context for this change. Why no PR description and no commit message body?
>
> I put it in the second comment. Edited and moved to the description.

Ok, I did a bit more digging: I was missing the context that https://github.com/bitcoin/bitcoin/pull/29765 upgraded the CI images to a newer Ubuntu version and that the `Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT` job [is now failing](https://github.com/bitcoin/bitcoin/pull/29786/checks?check_run_id=23346486009)
...
💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2033997944)
> > is the desire here to have the unit tests run by default when running individual tests
>
> No
>
> > (and also by default when running all tests)?
>
> Yes
>
> They are just another test now, so they don't need special treatment via `--skipunit`. Conversely, they don't need to be run more often than other tests.
>
> > The user can specify --failfast to end testing early if any test fails (unit tests are no exception)
>
> Yes, that seems good enough.
>

Ah, thank you for clarifying.
@mzu
...
💬 hebasto commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2034000240)
> Is it an option to drop the USDT tests from the `Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT` job and move them to a GH actions 23.04 VM if "lifting the requirements from the self-hosted runners" is the general direction we want to go? (probably makes sense to merge this sooner than later before adding a GH actions VM job to have CI green again?).

I'll look into it.
💬 mzumsande commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2034005144)
> utxo set snapshots are useful for more than just snapshot loading.

Do you know of any examples of the utxo dumps already being used for something else?
💬 maflcko commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2034006509)
> Ok, I did a bit more digging: I was missing the context that #29765 upgraded the CI images to a newer Ubuntu version and that the `Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT` job [is now failing](https://github.com/bitcoin/bitcoin/pull/29786/checks?check_run_id=23346486009).

29765 didn't do the switch, it was done in fa83b65ef8934b44fbac02da8dbc27fc0bc230e6 last year.



> Is it an option to drop the USDT tests from the `Cirrus CI / ASan + LSan + UBSan + integer, no dep
...
💬 0xB10C commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2034008362)
> GH actions 23.04 VM

GitHub currently only supports: `ubuntu-latest` (22.04), `ubuntu-22.04`, and `ubuntu-20.04`

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories
👍 fanquake approved a pull request: "build, depends: Fix `libmultiprocess` cross-compilation"
(https://github.com/bitcoin/bitcoin/pull/29665#pullrequestreview-1976129823)
ACK 2de2ea2ff63b97eacb23234932c6e1f1f65e4494 - I checked that this fixes the macOS cross-compilation issue. I'm assuming these packages are also likely to change further in the (near) future, given the changes going in upstream: https://github.com/chaincodelabs/libmultiprocess/pulls?q=is%3Apr+is%3Aclosed.

> I don't actually understand why the fix is needed, when this was broken, or why mpgen linking should fail when it is cross compiled.

I agree with Russ that it'd be better to actually f
...
💬 mzumsande commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2034027583)
> @mzumsande, now that unit tests are being run in parallel, we could treat them like any other test (i.e. no special treatment with `--skipunit`, so this option could now be removed). Do you see this aligning with the original goals of `skipunit` or is there something else to consider?

Yes, if they are treated like other tests, `-skipunit` is no longer necessary and should be removed. That's even better than the status quo because it removes the need to remember that `-skipunit` exists when
...
🚀 fanquake merged a pull request: "build, depends: Fix `libmultiprocess` cross-compilation"
(https://github.com/bitcoin/bitcoin/pull/29665)
👍 fanquake approved a pull request: "[25.x] Finalize 25.2"
(https://github.com/bitcoin/bitcoin/pull/29794#pullrequestreview-1976147858)
ACK e95c484f7de7fe30d5a2e87c0e7e2a5faf9c1a5f
🚀 fanquake merged a pull request: "[25.x] Finalize 25.2"
(https://github.com/bitcoin/bitcoin/pull/29794)
🚀 fanquake merged a pull request: "depends: add -g to DEBUG=1 flags"
(https://github.com/bitcoin/bitcoin/pull/29527)
💬 fanquake commented on pull request "depends: add -g to DEBUG=1 flags":
(https://github.com/bitcoin/bitcoin/pull/29527#issuecomment-2034083503)
Going to followup with an optimization flags alingment RFC.
💬 glozow commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2034094652)
(Yay v26.1) I have a couple more things for the last section, and then will take out of draft.
💬 fanquake commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2034145974)
> IIRC usage was turned into a compile error (maybe only intermittently, will check again).

I think I was thinking of something else here. Have changed this to just adding the new macro.