💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565716163)
Yes. Then the `last_confirmed` in the priority will default to the epoch time.

📝 Ataraxia009 opened a pull request: "init: clean up loadchainstate function"
(https://github.com/bitcoin/bitcoin/pull/33953)
`LoadChainState` had some redundant code, more specifically:
- unnecessarily setting the `m_chain` variable
- recalling `GetBestBlock()` unnecessarily

✅ Ataraxia009 closed a pull request: "init: clean up loadchainstate function"
(https://github.com/bitcoin/bitcoin/pull/33953)

💬 fanquake commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3582235905)
Backported to `30.x` in #33609.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565803681)
> if this is smaller than other, return greater?

Yes, because "Smaller num_picked ... mean greater priority". I think it is natural to have `this` on the left hand side and `other` on the right of the comparison.

These store a point in time, not number of seconds (interval length). To me it looks that `NodeClock::time_point` is the correct type for that. "Point in time" might be implemented as number of seconds since epoch, but this is internal to the std library.

I prefer to separate i
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565815751)
Both `*foo` and `foo.value()` are valid. I find the latter more readable and prefer to keep it. Same with `if (foo)` vs `if (foo.has_value())`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565823670)
Right! Done.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565840524)
Might as well pass the number to the constructor, but I think that is ungrounded. Sometimes it will benefit because it will do less expansions, but sometimes it will allocate unnecessary more. For this case in particular where we don't store many elements and is not called frequently (aka performance is not critical) I find the prevailing factor to be to have less code.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565842397)
Both seem to be valid. I prefer the current one.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565845046)
I think it is better to have it as a comment.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565856458)
Like in separate commits?

I have this in my `~/.gitconfig`:
```gitconfig
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565864221)
one is moved up, the other is down. We need to check that the code in-between doesn't mutate their dependencies - it's not about diffs, those are trivial to check, but making sure the state of the world doesn't change is the tough part.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565870274)
> This is redundant

Yes, if the transaction is valid. This code is fed transactions from outside and does not do own validation, so it cannot be sure that the caller has validated. I slightly incline towards keeping it as it is because it is harmless.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3582423083)
`12dfb1cb7c...50c8320f20`: take some suggestions and minor tweaks

💬 hebasto commented on pull request "ci: Run GUI unit tests in cross-Windows task":
(https://github.com/bitcoin/bitcoin/pull/33919#issuecomment-3582434851)
> Looks like the output is missing...

The same happens for the native build: https://github.com/hebasto/bitcoin/actions/runs/19707163063/job/56457637457.

It seems to be a peculiarity of the GHA Windows image.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565904681)
> Yes, if the transaction is valid.

Interesting. How can an invalid tx have different txids but same wtxids?

🤔 hebasto reviewed a pull request: "depends: update freetype and document remaining `bitcoin-qt` runtime libs"
(https://github.com/bitcoin/bitcoin/pull/33952#pullrequestreview-3512059986)
Concept ACK.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565911693)
No idea, but this way of keeping both we don't have to find out :)

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565916767)
Hmm, it cannot, I was confused. Will drop the txid comparison.

🤔 hebasto reviewed a pull request: "guix: reduce allowed exported symbols"
(https://github.com/bitcoin/bitcoin/pull/33950#pullrequestreview-3512084049)
Concept ACK.