💬 jonatack commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2112930271)
@stratospher Referencing the review feedback in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145326660, the argument about complexity doesn't outweigh that, and less so when there are simple alternatives that have been proposed. If this were to be merged as-is while ignoring the review feedback above, then we'd need to propose fixes for it. I don't think it makes sense to break things only to need to fix them.

💬 m3dwards commented on issue "ci: Enable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/issues/29804#issuecomment-2112930661)
> Is someone interested in moving the asan task over to GHA now?

Happy to look at this early next week.

💬 stickies-v commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1601905351)
You could do this by inspecting the logs?

<details>
<summary>git diff on 54cb1abffa</summary>

```diff
diff --git a/test/functional/rpc_getversion.py b/test/functional/rpc_getversion.py
index 712c1f76fc..366ca378e9 100755
--- a/test/functional/rpc_getversion.py
+++ b/test/functional/rpc_getversion.py
@@ -4,6 +4,8 @@
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test the getversion RPC."""

+import re
+
from test_framework.test_framework import Bitc
...

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601903709)
This call invokes logging, i.e. I/O operations, while holding the `::cs_main` and `m_mempool->cs` mutexes. It is considered as a suboptimal approach from the performance point of view. Could it be avoided?

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601905404)
Is the `mutable` keyword really required?

💬 jonatack commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2112936075)
> simply [reports an error](https://github.com/bitcoin/bitcoin/blob/aebfac13443c2abf78ebd88cd1f41213ca79ce5a/src/bitcoin-cli.cpp#L273) in case an older version of bitcoind is used with a newer bitcoin-cli

That's not helpful to someone running an older node (say, for benchmarking purposes, that might also include compiling data from -addrinfo) that they want to call with the latest version of bitcoind. You'd be simply breaking it for them needlessly.

📝 Mmgg002 opened a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/30113)
<!--
*** 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-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improve coverage are a
...

✅ hebasto closed a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/30113)

📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/30113)
<!--
*** 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-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improve coverage are a
...

💬 maflcko commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2112954537)
The windows failure is:

```
D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601920676)
I don't think it's true that `m_mempool->cs` is still held here (see line above)?

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601923988)
> I don't think it's true that `m_mempool->cs` is still held here (see line above)?

You're right. I was confused by indentation. My apologies.

👋 Mmgg002's pull request is ready for review: "."
(https://github.com/bitcoin/bitcoin/pull/30113)

💬 hebasto commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2112971987)
> The windows failure is:
>
> ```
> D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]
> ```

Should fixed with:
```diff
--- a/build_msvc/bitcoin_config.h.in
+++ b/build_msvc/bitcoin_config.h.in
@@ -11,6 +11,9 @@
/* Version is release */
#define CLIENT_VERSION_IS_RELEASE $

+/* Release candidate */
+#define CLIENT_VERSION_RC $
+
/* Major version */

...

💬 apoelstra commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2112984373)
Definite concept ACK from me.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601946313)
As for `cs_main`, it seems necessary for the chain tip to be what we think it is when the callback happens. IIUC the logging only happens in debug?

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2113010122)
> > Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?
>
> I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

I tracked recent 1000 blocks from block `842457` to `843457`

~61% of transactions in the last 1000 blocks were confirmed in a cluster size > 1.
~38% of transactions in the last 1000 blocks were confirmed in a cluster size 1

Transactions: 3974143
Cluster size 1 transactions: 1516505
Approximate percenta
...

💬 BrandonOdiwuor commented on pull request "Add used balance to overview page":
(https://github.com/bitcoin-core/gui/pull/775#issuecomment-2113012373)
@hebasto Sorry I will be updating the PR description

There was a change in implementation. I created a different PR (https://github.com/bitcoin/bitcoin/pull/29062) based on the recommendation to refactor the `getbalance()` function which touches the bitcoin repo. trying to get it merged so that I can update this PR

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2113026149)
Thanks for review @rkrux
- I addressed all your comments
- Force pushed from c12a677cc250608171bc4f6311095b60ba24abab to 2563305c0aef3975a6321911db7e0f2a245486de
[compare diff](https://github.com/bitcoin/bitcoin/compare/c12a677cc250608171bc4f6311095b60ba24abab..2563305c0aef3975a6321911db7e0f2a245486de)

💬 hebasto commented on pull request "Add used balance to overview page":
(https://github.com/bitcoin-core/gui/pull/775#issuecomment-2113028202)
> There was a change in implementation. I created a different PR ([bitcoin/bitcoin#29062](https://github.com/bitcoin/bitcoin/pull/29062)) based on the recommendation to refactor the `getbalance()` function which touches the bitcoin repo. trying to get it merged so that I can update this PR

Okay. Until then, marking this PR as a draft.