💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565696859)
I prefer a programming style where things are rather on separate lines instead of combining things into one line. But `try_emplace()` is a clear winner here over `emplace()`, taken.

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

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565701315)
Outdated, no more after using `max_element()`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565703911)
Outdated, no more after using `max_element()`.

📝 fanquake opened a pull request: "depends: update freetype and document remaining `bitcoin-qt` runtime libs"
(https://github.com/bitcoin/bitcoin/pull/33952)
Update freetype to `2.11.1`.
Updating fontconfig (currently `2.12.6`) to `2.13.1` requires what looks like a hard dep on gperf; leaving that as-is for now. If someone wants to investigate,
Document expectations in `symbol-check.py`.
Closes #29977 (and changes based on discussion there).

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2565710172)
Rough overview, but reworded as suggested.

💬 fanquake commented on issue "depends: Freetype and xcbproto version in depends are too new":
(https://github.com/bitcoin/bitcoin/issues/29977#issuecomment-3582195905)
See #33952.

💬 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.