💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601741212)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601581136

> Would have gone the opposite way and called it `throw_errors` since it is an.. exception.. to maintain legacy behavior. Sorry for not catching that earlier.

I think either way is fine, but catch_errors does seem more literally correct since the argument is just controlling whether the exceptions will be caught. Errors will be still be thrown regardless.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2112708036)
@willcl-ark, @tdb3, any interest in re-acking? This seems like it could definitely be merged with another ack

👋 furszy's pull request is ready for review: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574)

💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601790030)
> I was just re-using the pattern used for the `ChainstateManager` construction, but I like this more. Taking.

Oh, that's interesting. Looking at that, I can see how it's better than what I suggested because it allows callers to potentially move options without copying them at all. Current init code does copy the options because it is looping, but we don't need to require other callers to copy as well.

So you might want to keep this code like it is, though I would still suggest combining e
...

💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2112753291)
> @willcl-ark, @tdb3, any interest in re-acking? This seems like it could definitely be merged with another ack

Definitely. I'll plan to take a look tonight.

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