💬 willcl-ark commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2112786215)
You benchmark looks good and reports the result I'd expect too!

I'll investigate my own measurements some more in the mean time...

📝 0xB10C opened a pull request: "rpc: introduce getversion RPC"
(https://github.com/bitcoin/bitcoin/pull/30112)
The Bitcoin Core RPC interface [is implicitly versioned on the major version of Bitcoin Core](https://github.com/bitcoin/bitcoin/blob/42d5a1ff25a8045b6f4c09fa1fb71736dbccc034/doc/JSON-RPC-interface.md?plain=1#L67-L68). Some downstream RPC consumers and clients, for example rust-bitcoincore-rpc, need to know about the underlying node version to determine the available RPC calls and how to interpret the RPC responses (e.g. https://github.com/rust-bitcoin/rust-bitcoincore-rpc/issues/355).

The [
...

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1601825425)
> AFAICT the operations executed while the `reindexing` atomic is true do not issue any validation signals, so they don't have an effect on the indexes.

I need to test this, but this would be surprising to me. I think the idea behind having a separate StartIndexBackgroundSync function that runs after index initialization is that indexes should be able to receive signals during reindexing when `-reindex` is used so they do not need to completely resync when indexing is finished.

Will check
...

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