💬 willcl-ark commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2565201088)
Not relevant any more since e07e57368e9fab8ecfc140d44aef7db9b23c7ce0

💬 willcl-ark commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2565201930)
removed in e07e57368e9fab8ecfc140d44aef7db9b23c7ce0

💬 maflcko commented on pull request "Clear out space on CentOS, depends, gui GHA job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3581553019)
lgtm ACK e07e57368e9fab8ecfc140d44aef7db9b23c7ce0

Missing `ci: ` prefix in the title?

💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#issuecomment-3581563308)
> In PR description:
>
> ```diff
> - primarily adds test coverage for some of the most prominent failure cases in the first commit.
> + primarily adds test coverage for some of the most prominent failure cases in the last commit.
> ```

Fixed as well

✅ maflcko closed an issue: "Docs: "External signer" documentation is outdated. (plz close if unwanted request)"
(https://github.com/bitcoin/bitcoin/issues/31005)

💬 maflcko commented on issue "Docs: "External signer" documentation is outdated. (plz close if unwanted request)":
(https://github.com/bitcoin/bitcoin/issues/31005#issuecomment-3581571135)
ok, let's move the discussion to the pull request.

💬 maflcko commented on pull request "doc: update interface, --stdin flag, `signtx` (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#issuecomment-3581579197)
Please use unique, descriptive commit message, or squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits.

Also, there are a few LLM linter comments. Not sure if they all apply, but the missing `a` article makes sene.

💬 fanquake commented on pull request "policy: Remove individual transaction <minrelay restriction":
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2565262574)
LLM: s/includeing/including/

💬 maflcko commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3581633296)
Not sure. Is this caused by removing `-g`? If yes, I am not sure this is something meaningful to support when running with sanitizers.

💬 maflcko commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3581643069)
Going further, we could require `-fno-inline` for ubsan. But if you want to go the route in this pull, might as well just wildcard-suppress `InsecureRandomContext`?

💬 fjahr commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#discussion_r2565291756)
Ah, neat, I am sure I knew `num_addresses` existed. Updated the code to use it now and confirmed the result is the same as before for a diff between two different asmap files.

🚀 fanquake merged a pull request: "interfaces: remove redundant mempool lock in ChainImpl::isInMempool()"
(https://github.com/bitcoin/bitcoin/pull/33946)

🤔 l0rinc reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3502433168)
Thanks for being so responsive and open to suggestions!
Publishing my intermediary review to speed up the response cycle.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564625280)
I think a `try_emplace` is more fitting here to avoid creation in case it already exists - it also allows us to avoid the default-constructed send-status list:
```suggestion
const bool inserted{m_transactions.try_emplace(tx).second};
```
or
```C++
LOCK(m_mutex);
return m_transactions.try_emplace(tx).second;
```
or even
```C++
return WITH_LOCK(m_mutex, return m_transactions.try_emplace(tx).second);
```

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564486363)
nit: I don't think it's fair to call it a list. Resolve without comment if irrelevant.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564656833)
we're storing this redundantly, it should be already available to us via `most_urgent_it->first`

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2557883037)
nit: this non-purity (`NodeClock::now()`) can make testing and benchmarking a bit tougher - consider passing it as a param. Just resolve if you disagree.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564687076)
what should happen when a tx was never confirmed, are we deliberately classifying those as stale?

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564662338)
instead of checking whether `most_urgent` is assigned at every iteration, we could init `most_urgent_it` with the first value and do the loop starting from `auto it{std::next(m_transactions.begin())}.
If it's possible to call this with an empty tx list, we could guard it at the beginning.
```C++
LOCK(m_mutex);

if (m_transactions.empty()) return std::nullopt;

auto most_urgent_it{m_transactions.begin()};
Priority most_urgent_prio{DerivePriority(most_urgent_it->second)};
for (auto it{std
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564673247)
is this still accurate (or it's just meant to be a rough overview?)
```suggestion
* Pick the transaction with the fewest send attempts, and confirmations, and oldest send/confirm times.
```