Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 l0rinc commented on something "":
(https://github.com/bitcoin/bitcoin/commit/73fe7d723084653671f2178ea1177a8627edfafa#r146619899)
nit: this introduced a lint-spelling warning:
> % test/lint/lint-spelling.py
src/ipc/protocol.h:57: neccessary ==> necessary
💬 l0rinc commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1756962957)
nit: this introduced a lint-spelling warning:
> % test/lint/lint-spelling.py
src/ipc/protocol.h:57: neccessary ==> necessary
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2346432017)
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678

The change progressed a lot from its first version and based on the enthusiasm among reviewers I'm looking forward to continuing these in follow-up PRs.
📝 fjahr opened a pull request: "test: Wait for local services to update in feature_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/30880)
Closes #30878

It seems like there is a race where the block is stored locally and `getblock` does not error anymore, but `ActivateBestChain` has not finished yet, so the local services are not updated yet either. Fix this by waiting for the local services to update.
💬 maflcko commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#issuecomment-2346436631)
re-review ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8

only change is a comment
💬 achow101 commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2346444421)
Discussion from today's IRC meeting is that we should move forward with @davidgumberg's patch and default to false on Windows for this release.
💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346447243)
(Let's merge this minimal bugfix first, and then change the output in a follow-up. Otherwise, this will be delayed and running the s390x will not be possible out of the box)
🤔 pablomartin4btc reviewed a pull request: "test: Wait for local services to update in feature_assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/30880#pullrequestreview-2300493751)
ACK d823f59c91cb00fe763b0c036454440f1948bf48

Perhaps also at this line L701 too:

`assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])`

(It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)
💬 fjahr commented on pull request "test: Wait for local services to update in feature_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346457532)
> Perhaps also at this line L701 too:
>
> `assert {'NETWORK', 'NETWORK_LIMITED'}.issubset(n2.getnetworkinfo()['localservicesnames'])`
>
> (It also errors there: https://cirrus-ci.com/task/5740917921939456?logs=ci#L3362)

Yeah, I just saw that after I succeeded to reproduce it locally, so don't merge yet please.
👍 fanquake approved a pull request: "build: Skip secp256k1 ctime tests when tests are not being built"
(https://github.com/bitcoin/bitcoin/pull/30865#pullrequestreview-2300503583)
ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8
🚀 fanquake merged a pull request: "build: Skip secp256k1 ctime tests when tests are not being built"
(https://github.com/bitcoin/bitcoin/pull/30865)
💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756990148)
But we don't need `bool` for dirty anymore, only for freshness - so we can even get rid of the enum (moving it over to the test maybe). I'll experiment to see what the next smallest change is before this PR, we need to have tests that are more representative of what's actually possible.
🚀 fanquake merged a pull request: "test: fix exclude parsing for functional runner"
(https://github.com/bitcoin/bitcoin/pull/30872)
💬 fjahr commented on pull request "test: Wait for local services to update in feature_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346486397)
Ok, `feature_assumeutxo.py` should be good now with this, ready for review.
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757005855)
9378b30774d40c8a093c69d2b1807c9eea32455a: I think you need to wrap this in `if(BUILD_TESTS)`
💬 antonilol commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2346494798)
> ### [minimal_key_value](https://github.com/bitcoin/bitcoin/compare/20b259332ff1d2e6ca6b291ffeda00b51497bbed...hodlinator:bitcoin:pr/24539_minimal_key_value) / [ced5058](https://github.com/bitcoin/bitcoin/commit/ced505834dd1349bccbceeac3bf5abaa2327bda0)
>
> Minimize key & value footprint
>
> ...

This one (the third) is very similar to the 'spending' index of electrs (https://github.com/romanz/electrs), only different being how the key is calculated, electrs uses the first 8 bytes of th
...
💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#discussion_r1757015334)
Another way this would have been caught is by making warnings in the CI fatal. There is `ail_on_warn=args.ci` which can be used.
💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2346507315)
> > What if you ccache --zero-stats after configuring for a new build tree, just before building?
>
> In that case on Fedora it is `Hits: 446 / 446 (100.0%)`, with `Uncacheable calls: 12 / 458 ( 2.62%)`.

If `ccache --show-stats --verbose` outputs:
```
Uncacheable calls: 12 / 458 ( 2.62%)
Called for linking: 12 / 12 (100.0%)
```

this is expected on systems where `ccache` masquerades as a compiler.
💬 maflcko commented on pull request "test: Wait for local services to update in feature_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/30880#issuecomment-2346509249)
review-only ACK 19f4a7c95a99162122068d4badffeea240967a65

I did not test the reproducer.
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1757015032)
nit: this one line doesn't need to be changed and have the brace moved.