💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601827728)
> Flatten avoid an unnecessary move:

What do you mean with this?

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601841903)
It seems reasonable to prepend this function body with `AssertLockHeld(m_tx_download_mutex);` now, no?

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1601844115)
I was thinking that `i` means the key, as it is the symbol passed to the function.

Clarified in the latest push, by renaming `i` to `key`.

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1601845279)
Thanks, pushed your diff.

💬 stickies-v commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2112863394)
Concept ACK

🤔 marcofleon reviewed a pull request: "cli: Detect port errors in rpcconnect and rpcport"
(https://github.com/bitcoin/bitcoin/pull/29521#pullrequestreview-2058386154)
re-ACK e208fb5d3bea4c1fb750cb0028819635ecdeb415. I successfully compiled and built (`make check` passed) on my Arm64 machine but the CI error is related to 32-bit machines if I'm not mistaken so might not be relevant.

Either way, the port error detection looks good to me. Clear comments and more readable since my last review. I also successfully ran the functional test. I modified the test using valid ports and mixed `-rpcconnect` and `-rpcport` configurations to ensure robustness. The test
...

💬 stratospher 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#discussion_r1601872654)
liked it, done.

💬 stratospher 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-2112890762)
Thank you for the reviews @pablomartin4btc! I've rebased the PR and included your [suggestion](https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1360075599).

> Also, regarding both the totals and per network differences between before and after this changes I wonder if we should add some context in the output or in the help for the user to understand where those differences are coming from.

i prefer not adding niche details in the user output/help because the difference would only
...

💬 stratospher 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-2112892788)
> What is the state of this @stratospher? Are you still looking for feedback on #26988 (comment)?

@sr-gi, yes! that's the only unresolved conversation. i personally prefer the current approach as explained in https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173966799.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601878465)
I missed deleting that. Fixed, thanks!

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601878592)
added

💬 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#discussion_r1601889565)
I don't think there is much complexity, nor is it a valid argument for breaking several years of context, particularly when there are non-difficult alternatives (proposed above).

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