🤔 marcofleon reviewed a pull request: "p2p: TxOrphanage revamp cleanups"
(https://github.com/bitcoin/bitcoin/pull/32941#pullrequestreview-3084354759)
Code review ACK 3d4d4f0d92d42809e74377e4380abdc70f74de5d
Fuzzed all three targets over the weekend, no crashes. Only small thing I caught while reading the `txorphan_protected` target:
https://github.com/bitcoin/bitcoin/blob/3d4d4f0d92d42809e74377e4380abdc70f74de5d/src/test/fuzz/txorphan.cpp#L331
That line should be `(tx->vin.size() / 10) + 1` but it doesn't result in a crash because it's overestimating the latency score for an additional announcer.
(https://github.com/bitcoin/bitcoin/pull/32941#pullrequestreview-3084354759)
Code review ACK 3d4d4f0d92d42809e74377e4380abdc70f74de5d
Fuzzed all three targets over the weekend, no crashes. Only small thing I caught while reading the `txorphan_protected` target:
https://github.com/bitcoin/bitcoin/blob/3d4d4f0d92d42809e74377e4380abdc70f74de5d/src/test/fuzz/txorphan.cpp#L331
That line should be `(tx->vin.size() / 10) + 1` but it doesn't result in a crash because it's overestimating the latency score for an additional announcer.
💬 sipa commented on pull request "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`":
(https://github.com/bitcoin/bitcoin/pull/33132#discussion_r2251601762)
Nit: if this is actually aiming to be forward compatible with a future more-than-two-levels extension (which we may in fact need at some point), I think this should be `sims.back(). ... = ...`. Doesn't matter now of course as it's all equivalent to level 0.
(https://github.com/bitcoin/bitcoin/pull/33132#discussion_r2251601762)
Nit: if this is actually aiming to be forward compatible with a future more-than-two-levels extension (which we may in fact need at some point), I think this should be `sims.back(). ... = ...`. Doesn't matter now of course as it's all equivalent to level 0.
💬 sipa commented on pull request "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`":
(https://github.com/bitcoin/bitcoin/pull/33132#issuecomment-3150855612)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
(https://github.com/bitcoin/bitcoin/pull/33132#issuecomment-3150855612)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
✅ glozow closed an issue: "fuzz: `txgraph`: Assertion `cmp == 0' failed"
(https://github.com/bitcoin/bitcoin/issues/33097)
(https://github.com/bitcoin/bitcoin/issues/33097)
🚀 glozow merged a pull request: "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`"
(https://github.com/bitcoin/bitcoin/pull/33132)
(https://github.com/bitcoin/bitcoin/pull/33132)
📝 0xB10C opened a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133)
The docs have been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L249-L257
<!--
*** 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/bitcoin/pull/33133)
The docs have been incorrect since a3789c700b5a43efd4b366b4241ae840d63f2349 (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L249-L257
<!--
*** 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
...
💬 0xB10C commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3150986125)
It's correctly documented in the `ping` RPC docs https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3150986125)
It's correctly documented in the `ping` RPC docs https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88
💬 maflcko commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3151021429)
lgtm ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3151021429)
lgtm ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
💬 willcl-ark commented on pull request "ci: Use mlc `v1` and ignore `depends/work`":
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251648192)
Yes, the `--gitignore` flag is slightly buggy. I have fix this (and speeded up the entire run) by ignoring all files in ignore directores in this patch: https://github.com/becheran/mlc/compare/master...willcl-ark:mlc:ignore-dirs-find
Running that branch, with a dirty depends dir fixes that issue for me, although I still see one incorrect "error":
```
Result (1871 links):
OK 458
Skipped 1057
Warnings 355
Errors 1
The following links could not be resolved:
./src/secp2
...
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251648192)
Yes, the `--gitignore` flag is slightly buggy. I have fix this (and speeded up the entire run) by ignoring all files in ignore directores in this patch: https://github.com/becheran/mlc/compare/master...willcl-ark:mlc:ignore-dirs-find
Running that branch, with a dirty depends dir fixes that issue for me, although I still see one incorrect "error":
```
Result (1871 links):
OK 458
Skipped 1057
Warnings 355
Errors 1
The following links could not be resolved:
./src/secp2
...
💬 fanquake commented on pull request "ci: Use mlc `v1` and ignore `depends/work`":
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251720043)
> I suppose we should explicitly ignore our subtrees too then...
Aren't we already with `let mut md_ignore_paths = get_subtrees();` ?
(https://github.com/bitcoin/bitcoin/pull/33125#discussion_r2251720043)
> I suppose we should explicitly ignore our subtrees too then...
Aren't we already with `let mut md_ignore_paths = get_subtrees();` ?
💬 0xB10C commented on pull request "[29.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33074#issuecomment-3151100176)
#33133 probably too
(https://github.com/bitcoin/bitcoin/pull/33074#issuecomment-3151100176)
#33133 probably too
💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3151106136)
> That line should be (tx->vin.size() / 10) + 1
Thanks @marcofleon! Just added a commit on top to fix that.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3151106136)
> That line should be (tx->vin.size() / 10) + 1
Thanks @marcofleon! Just added a commit on top to fix that.
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3151110988)
Hmm, so I looked into re-running GHA CI tasks to catch silent merge conflicts. However, GitHub does not allow this and blocks any re-run if the commit is older than 30 days. Even if the task has been recently re-run. Example: https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/47185933876?pr=32856 (can not be re-run anymore)
If the ability to re-run CI tasks is removed without replacement, this will likely lead to more pull requests being merged into master, where the CI fails.
The
...
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3151110988)
Hmm, so I looked into re-running GHA CI tasks to catch silent merge conflicts. However, GitHub does not allow this and blocks any re-run if the commit is older than 30 days. Even if the task has been recently re-run. Example: https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/47185933876?pr=32856 (can not be re-run anymore)
If the ability to re-run CI tasks is removed without replacement, this will likely lead to more pull requests being merged into master, where the CI fails.
The
...
🤔 jonatack reviewed a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3084685673)
ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3084685673)
ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
💬 jonatack commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#discussion_r2251809033)
nit, unsure if the "(s)" mentions improve anything now, maybe omit them
(https://github.com/bitcoin/bitcoin/pull/33133#discussion_r2251809033)
nit, unsure if the "(s)" mentions improve anything now, maybe omit them
🤔 jonatack reviewed a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3084689548)
ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3084689548)
ACK 12df1a72831bf950c5d625b80c6fd6ef15b4117d
💬 Crypt-iQ commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#discussion_r2251819585)
I get that if this is an orphan, we only have the parent txids. In most cases, it seems like this reconsiderable filter will be storing wtxid? The exceptions being a non-segwit tx and if a parent is TX_RECONSIDERABLE but is actually witness-stripped (I'm not sure if there are others?). I may be missing something here, are there more cases where this reconsiderable filter check be used with txid?
(https://github.com/bitcoin/bitcoin/pull/33066#discussion_r2251819585)
I get that if this is an orphan, we only have the parent txids. In most cases, it seems like this reconsiderable filter will be storing wtxid? The exceptions being a non-segwit tx and if a parent is TX_RECONSIDERABLE but is actually witness-stripped (I'm not sure if there are others?). I may be missing something here, are there more cases where this reconsiderable filter check be used with txid?
💬 jonatack commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3151218121)
> It's correctly documented in the `ping` RPC docs
>
> https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88
Would it make sense to add "minping" here?
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3151218121)
> It's correctly documented in the `ping` RPC docs
>
> https://github.com/bitcoin/bitcoin/blob/0cb1ed2b7c634754f4ad0a0083d7cf40989d7f48/src/rpc/net.cpp#L85-L88
Would it make sense to add "minping" here?
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2251833326)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2251833326)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2251834983)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2251834983)
Done