👍 maflcko approved a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2331072265)
lgtm
Thanks for taking the feedback
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2331072265)
lgtm
Thanks for taking the feedback
💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1776965132)
nit in ed57f6ee02d5a6ada23abaea6e2176cd4f173e4f: Would be nice to do it in tinyformat itself, or not at all. ConstevalFormatString is already checked, so extremely unlikely to hit. However, translated strings (which are apparently unreviewed, see https://github.com/bitcoin/bitcoin/issues/30897) are left unchecked and may throw.
It would be good to validate and review the translated strings, or remove translations completely for now, but this is probably a separate issue.
Here, it would be
...
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1776965132)
nit in ed57f6ee02d5a6ada23abaea6e2176cd4f173e4f: Would be nice to do it in tinyformat itself, or not at all. ConstevalFormatString is already checked, so extremely unlikely to hit. However, translated strings (which are apparently unreviewed, see https://github.com/bitcoin/bitcoin/issues/30897) are left unchecked and may throw.
It would be good to validate and review the translated strings, or remove translations completely for now, but this is probably a separate issue.
Here, it would be
...
💬 Sjors commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376805725)
I would also prefer it if `bitcoind` (and `bitcoin-qt`) had multiprocess built in. That way the instruction for miners is "Install Bitcoin Core as you always do, just add `ipcbind=linux` to you config". cc @ryanofsky
> that should be fixed in depends, i.e (for now) don't even try building the packages/functionality for that platform, not leaked into Guix with a workaround.
I'll look into that.
> there's a number of other things that need to happen first. We should be switching multipro
...
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376805725)
I would also prefer it if `bitcoind` (and `bitcoin-qt`) had multiprocess built in. That way the instruction for miners is "Install Bitcoin Core as you always do, just add `ipcbind=linux` to you config". cc @ryanofsky
> that should be fixed in depends, i.e (for now) don't even try building the packages/functionality for that platform, not leaked into Guix with a workaround.
I'll look into that.
> there's a number of other things that need to happen first. We should be switching multipro
...
💬 ryanofsky commented on pull request "guix: add multiprocess binaries":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376836413)
It's good to have this PR open as a draft to see what it looks like but it sounds like there are a number of followups that need to happen for it to be ready. For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term. It would also be good to review and merge #30940 which this is currently based on.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2376836413)
It's good to have this PR open as a draft to see what it looks like but it sounds like there are a number of followups that need to happen for it to be ready. For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term. It would also be good to review and merge #30940 which this is currently based on.
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2376861686)
Rebased 7b9aa0b6eb1126cb32da91f3c9d2df12325aa90c -> 2227afac0372358287fb879f3b0bd07fd653f3f8 ([`pr/mine.10`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.10) -> [`pr/mine.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.10-rebase..pr/mine.11)) after base PR's #30509 and #30510 were merged.
Marking ready for review
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2376861686)
Rebased 7b9aa0b6eb1126cb32da91f3c9d2df12325aa90c -> 2227afac0372358287fb879f3b0bd07fd653f3f8 ([`pr/mine.10`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.10) -> [`pr/mine.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.10-rebase..pr/mine.11)) after base PR's #30509 and #30510 were merged.
Marking ready for review
👋 ryanofsky's pull request is ready for review: "multiprocess: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437)
(https://github.com/bitcoin/bitcoin/pull/30437)
⚠️ Sjors opened an issue: "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0"
(https://github.com/bitcoin/bitcoin/issues/30978)
Tried on Intel macOS 15.0 (Xcode 16.0) and 13.7 (Xcode 15.2):
```
$ cd depends
$ make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump: No such file or directory
/bin/sh: command -v dsymutil: No such file or directory
Fetching libevent-2.1.12-stable.tar.gz from ...
```
It does seem to finish building dependencies, but I haven't
...
(https://github.com/bitcoin/bitcoin/issues/30978)
Tried on Intel macOS 15.0 (Xcode 16.0) and 13.7 (Xcode 15.2):
```
$ cd depends
$ make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump: No such file or directory
/bin/sh: command -v dsymutil: No such file or directory
Fetching libevent-2.1.12-stable.tar.gz from ...
```
It does seem to finish building dependencies, but I haven't
...
💬 tdb3 commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2376991344)
> Any v2only option should apply only to IPv4 and IPv6 peers, but not to Tor, I2P, CJDNS because the latter already have (stronger) MITM prevention and encryption.
A somewhat minor nuance, but seems worth mentioning:
Unless I'm missing something, Tor entry and exit nodes would see v1 traffic in the clear. If it's decided to proceed with the option, it should be clear to users that it's only clearnet, Tor/I2P would still allow v1, and e.g. using Tor isn't a cure for P2P protection (it really
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2376991344)
> Any v2only option should apply only to IPv4 and IPv6 peers, but not to Tor, I2P, CJDNS because the latter already have (stronger) MITM prevention and encryption.
A somewhat minor nuance, but seems worth mentioning:
Unless I'm missing something, Tor entry and exit nodes would see v1 traffic in the clear. If it's decided to proceed with the option, it should be clear to users that it's only clearnet, Tor/I2P would still allow v1, and e.g. using Tor isn't a cure for P2P protection (it really
...
📝 fjahr opened a pull request: "contrib: Update asmap link in seeds readme"
(https://github.com/bitcoin/bitcoin/pull/30979)
I am moving all my ASMap related repositories to an asmap org. While there is a redirect in place that works for now, GitHub doesn't guarantee that it will keep working in the long term. So we should still fix the links.
(https://github.com/bitcoin/bitcoin/pull/30979)
I am moving all my ASMap related repositories to an asmap org. While there is a redirect in place that works for now, GitHub doesn't guarantee that it will keep working in the long term. So we should still fix the links.
💬 sdaftuar commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377062209)
@instagibbs Sure, I'd agree with that.
Another way to think about it: let's say we scrapped rule 2 as proposed here. The resulting set of rules would have their own logical inconsistencies, specifically rule 6:
> The replacement transaction's feerate is greater than the feerates of all directly conflicting transactions.
Specifically, a large transaction that had lower feerate than one of its direct conflicts could be split into two -- a parent with super low feerate, and a child with a
...
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377062209)
@instagibbs Sure, I'd agree with that.
Another way to think about it: let's say we scrapped rule 2 as proposed here. The resulting set of rules would have their own logical inconsistencies, specifically rule 6:
> The replacement transaction's feerate is greater than the feerates of all directly conflicting transactions.
Specifically, a large transaction that had lower feerate than one of its direct conflicts could be split into two -- a parent with super low feerate, and a child with a
...
📝 marcofleon opened a pull request: "fuzz: fix bug in p2p_headers_presync harness"
(https://github.com/bitcoin/bitcoin/pull/30980)
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
(https://github.com/bitcoin/bitcoin/pull/30980)
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
👋 ryanofsky's pull request is ready for review: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409)
(https://github.com/bitcoin/bitcoin/pull/29409)
💬 maflcko commented on issue "win64-cross CI timeout after 2h in allocator_tests":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2377098663)
https://cirrus-ci.com/task/4822998216081408?logs=ci#L2609
So I guess it is not allocator tests.
Maybe a wine issue?
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2377098663)
https://cirrus-ci.com/task/4822998216081408?logs=ci#L2609
So I guess it is not allocator tests.
Maybe a wine issue?
📝 willcl-ark opened a pull request: "ci: add timestamps to cirrus jobs"
(https://github.com/bitcoin/bitcoin/pull/30981)
Currently, debugging where time is spent in the cirrus jobs feels annoying, e.g. trying to see where time may be spent in https://github.com/bitcoin/bitcoin/issues/30969
Enable timestamps in the logs for more information.
(https://github.com/bitcoin/bitcoin/pull/30981)
Currently, debugging where time is spent in the cirrus jobs feels annoying, e.g. trying to see where time may be spent in https://github.com/bitcoin/bitcoin/issues/30969
Enable timestamps in the logs for more information.
💬 willcl-ark commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2377147030)
I opened https://github.com/bitcoin/bitcoin/pull/30981 to enable cirrus log timestamps. They may help in shedding some more light on where these timeout jobs are spending time, although in this case it seems to be a deeper bug...
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2377147030)
I opened https://github.com/bitcoin/bitcoin/pull/30981 to enable cirrus log timestamps. They may help in shedding some more light on where these timeout jobs are spending time, although in this case it seems to be a deeper bug...
💬 maflcko commented on pull request "ci: add timestamps to cirrus jobs":
(https://github.com/bitcoin/bitcoin/pull/30981#issuecomment-2377165712)
review ACK f951f1fab258f782a88bb006b5ae4ea486705388
It may be a bit annoying when looking at functional test failures, which have a timestamp included and would then get a "wrong" timestamp, but this seems fine .
(https://github.com/bitcoin/bitcoin/pull/30981#issuecomment-2377165712)
review ACK f951f1fab258f782a88bb006b5ae4ea486705388
It may be a bit annoying when looking at functional test failures, which have a timestamp included and would then get a "wrong" timestamp, but this seems fine .
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1777269039)
I've improved the comments a bit.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1777269039)
I've improved the comments a bit.
👍 tdb3 approved a pull request: "ci: add timestamps to cirrus jobs"
(https://github.com/bitcoin/bitcoin/pull/30981#pullrequestreview-2331638443)
Code Review ACK f951f1fab258f782a88bb006b5ae4ea486705388
There are a few times I've wished these timestamps were present.
```
[14:35:19.384] 127/137 Test #132: wallet_crypto_tests .................. Passed 2.48 sec
[14:35:19.385] Start 137: db_tests
[14:35:19.572] 128/137 Test #137: db_tests ............................. Passed 0.19 sec
[14:35:19.993] 129/137 Test #131: spend_tests .......................... Passed 3.13 sec
[14:35:20.153] 130/137 Test #31: coins_
...
(https://github.com/bitcoin/bitcoin/pull/30981#pullrequestreview-2331638443)
Code Review ACK f951f1fab258f782a88bb006b5ae4ea486705388
There are a few times I've wished these timestamps were present.
```
[14:35:19.384] 127/137 Test #132: wallet_crypto_tests .................. Passed 2.48 sec
[14:35:19.385] Start 137: db_tests
[14:35:19.572] 128/137 Test #137: db_tests ............................. Passed 0.19 sec
[14:35:19.993] 129/137 Test #131: spend_tests .......................... Passed 3.13 sec
[14:35:20.153] 130/137 Test #31: coins_
...
💬 marcofleon commented on pull request "test: Add missing sync_mempools() to fill_mempool()":
(https://github.com/bitcoin/bitcoin/pull/30948#issuecomment-2377284536)
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe.
(https://github.com/bitcoin/bitcoin/pull/30948#issuecomment-2377284536)
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe.
🤔 marcofleon reviewed a pull request: "test: Add missing sync_mempools() to fill_mempool()"
(https://github.com/bitcoin/bitcoin/pull/30948#pullrequestreview-2331663285)
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe
Saw `AssertionError: 0.00001000 <= 0.00001000` on master.
On this branch:
```
TEST | STATUS | DURATION
mempool_limit.py | ✓ Passed | 6 s
mempool_package_rbf.py | ✓ Passed | 24 s
p2p_1p1c_network.py | ✓ Passed | 53 s
p2p_opportunistic_1p1c.py | ✓ Passed | 43 s
p2p_tx_download.py | ✓ Passed | 31 s
rpc_packages.py | ✓ Passed | 4 s
ALL | ✓ Pa
...
(https://github.com/bitcoin/bitcoin/pull/30948#pullrequestreview-2331663285)
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe
Saw `AssertionError: 0.00001000 <= 0.00001000` on master.
On this branch:
```
TEST | STATUS | DURATION
mempool_limit.py | ✓ Passed | 6 s
mempool_package_rbf.py | ✓ Passed | 24 s
p2p_1p1c_network.py | ✓ Passed | 53 s
p2p_opportunistic_1p1c.py | ✓ Passed | 43 s
p2p_tx_download.py | ✓ Passed | 31 s
rpc_packages.py | ✓ Passed | 4 s
ALL | ✓ Pa
...