💬 maflcko commented on pull request "refactor: Avoid -W*-whitespace in git archive":
(https://github.com/bitcoin/bitcoin/pull/33869#issuecomment-3526775978)
> Should we modify one job to use the archive?

Possibly, but I don't think any job is running with gcc-15, so the benefits here would be limited. Though, I can modify my nightly matrix to use archives explicitly, instead of by accident of git not being present.



> We should probably be catching, and minimising any differences, like this, between building from source, and the tarball.

I want to keep the changes here minimal. Reworking this is already tracked in https://github.com/bitc
...

💬 m3dwards commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#discussion_r2522600040)
Missing import time?

💬 m3dwards commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#discussion_r2522613366)
Should be `build_cmd`

💬 vasild commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3526831112)
`9989853447...78595ae0e7`: take suggestions

✅ fanquake closed an issue: "fees: leftover references to `policy/fees.cpp`"
(https://github.com/bitcoin/bitcoin/issues/33863)

🚀 fanquake merged a pull request: "scripted-diff: fix leftover references to `policy/fees.h`"
(https://github.com/bitcoin/bitcoin/pull/33864)

💬 vasild commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2522652086)
This slipped through, thanks for the reminder. Done now and also took the changes into https://github.com/bitcoin/bitcoin/pull/29415

💬 vasild commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2522653126)
Done

💬 vasild commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2522654868)
Done

💬 m3dwards commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3526866633)
The GHA behaviour of removing annotations from previous runs is very annoying! I experimented with having an upstream job that printed the annotation and having all the other jobs depend on it but again when you rerun a failed downstream job you lose the annotation from the run upstream job.

It's a shame that in this PR we have to have the annotation printed many times, one per job, as I think it might drown out other notices. Without knowing exactly how this machine readable annotation is be
...

💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#issuecomment-3526892334)
Updated 6d1818888b1e56dec6c57fadb3a99bfacbab742f -> d1823ebdd32d106668a3a8085da31e274d5c0ac1 ([batch_write_void_0](https://github.com/TheCharlatan/bitcoin/tree/batch_write_void_0) -> [batch_write_void_1](https://github.com/TheCharlatan/bitcoin/tree/batch_write_void_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/batch_write_void_0..batch_write_void_1))

* Addressed @andrewtoth's [comment](https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2520746299), use `NextAndMaybeEras
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3526942592)
`875795ef0f...7547f915bc`: take suggestions from https://github.com/bitcoin/bitcoin/pull/33565

💬 fanquake commented on pull request "refactor: Avoid -W*-whitespace in git archive":
(https://github.com/bitcoin/bitcoin/pull/33869#issuecomment-3527012975)
> Possibly, but I don't think any job is running with gcc-15, so the benefits here would be limited.

I'm just thinking generally, not about this single warning, or gcc-15. Happy for it to best changed in a followup, or in another related change.

💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2522793220)
Fixed.

💬 stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2522796090)
_response to https://github.com/bitcoin/bitcoin/pull/33855#issuecomment-3526237850_

@alexanderwiederin opening this thread for easier focus. Are you suggesting we 1) implement `btck_block_tree_entry_equals` with a hash equality check, or 2) to not expose `btck_block_tree_entry_equals` at all and let the user define it however they want (such as using hash equality), or 3) something else?

If 1), I don't see the benefit? Pointer comparisons are faster, and are conceptually correct, and what
...

💬 maflcko commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3527196104)
> It's a shame that in this PR we have to have the annotation printed many times, one per job, as I think it might drown out other notices.

Agree, but I think we can use warning notices to bubble up more important ones.



> Without knowing exactly how this machine readable annotation is being consumed or experimenting more, I'm not aware of an alternative?

It can be used via the API, e.g. https://github.com/maflcko/DrahtBot/blob/6c91378116516ca5974d78a4e0c5c495b38acf95/webhook_feature
...

💬 vasild commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3527310703)
Good question, I guess it is a bit obscure. Let me elaborate. So, when we receive the `VERSION` message, its handling (inside the `if (msg_type == NetMsgType::VERSION) {`) currently goes like this:

```
if (...)
A
if (...)
MakeAndPushMessage()
if (...)
B
if (...)
MakeAndPushMessage()
if (...)
some stuff that better be avoided for private broadcast connections
```

For private broadcast we don't want to push any extra messages. To do that I took the approach to
...

💬 vasild commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3527377529)
> What’s the rationale for moving `m_addrman.Good(pfrom.addr)` earlier ?

Hmm, wait! `m_addrman.Good()` need not be earlier, it is one of the "some stuff that better be avoided". Then this can be simplified? :bulb:

💬 ajtowns commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3527384125)
ACK aef40b93cf057d2a27d61881b0858d491206bcd3 -- matches what I was thinking

Adding some assertions in `Ref::Ref(Ref&&)` triggers when I run the fuzz binary over some txgraph corpus data I generated previously, so https://github.com/bitcoin/bitcoin/pull/33862#discussion_r2519205585 doesn't seem like a real problem.

👍 l0rinc approved a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3459390175)
I don't want to be dismissive of @ajtowns' claims. I agree they're all valid, and please allow me to push back respectfully.

> bikeshedding about the names of functions is not a good use of anyone's time here.

I've been tricked many times by misnamed methods. If you think the old name is good, or the new name is worse, or the new comment isn't adding anything, I can empathize with that. But nobody is forcing us to review this. I personally do it because I want some progress with the origin
...

💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2523145922)
the enum name aligns more closely with the method name 👍