💬 willcl-ark commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2117545232)
Concept ACK.

The code changes all look correct to me too, however

> @willcl-ark, however, noticed that memory usage may increase in some cases. Logically this makes no sense to me. The only plausible explanation imo is that because the moves are faster, more ops/second occur in some cases.

I still have not been able to explain why i see these type of results, but I think it's most likely user error somewhere along the line.

Here is one example of a puzzling result I have, in this tes
...

👋 BrandonOdiwuor's pull request is ready for review: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838)

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r1604968619)
I have updated the PR to use the `first 8 bytes of the hash160` and replaced the `-` with `_` as suggested

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2117559488)
@Sjors and @RandyMcMillan I love the idea of the `signet_XXXX` config section and would like to work on it if there is a lot of interest in a follow-up PR

💬 maflcko commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#issuecomment-2117574570)
rfm?

💬 ajtowns commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2117596305)
Would this make sense as part of `getrpcinfo` ? If the point of this is for RPC clients to check so they can be forwards/backwards compatible, then I'm not sure it really makes sense to look at anything other than the `numeric` field? Would the other fields make more sense as a response to a `getnodeinfo` command? (That would seem a more appropriate place for the `logpath` field `getrpcinfo` returns)

💬 tdb3 commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2117609243)
> > I'm partial to getnodeversion or getclientversion, since RPC is implicitly versioned by the major version of the client/node
>
> The problem with those, imo, is that `client` and `node` are both words associated with general networking. They might as well return P2P protocol version information. If this call is intended to return RPC versions and capabilities, then that should probably be explicit in the name and documentation. But maybe i'm misunderstanding the intent.
>
> > If in the fut
...

💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1605019621)
probably can assert this in the fuzz target?

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2117642767)
thanks for splitting this up, next on my review docket

👍 rkrux approved a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066#pullrequestreview-2063486974)
reACK [9365baa](https://github.com/bitcoin/bitcoin/pull/30066/commits/9365baa489e123d9bcaf986e4311d3fa3f1e3f88)

💬 maflcko commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2117664043)
I did test an earlier draft of this pull request and the only thing I noticed was that on a fresh desktop install of a Linux distro, sometimes a package would be missing, and the GUI would fail to start. If this is expected, I guess the docs should be adjusted to clarify that users would have to install the packages themselves?

💬 furszy commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2117677284)
CI failure is unrelated.

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1605053958)
Extra spaces after 0?

🤔 rkrux reviewed a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2063512473)
crACK [f9ba6fd](https://github.com/bitcoin/bitcoin/pull/29500/commits/f9ba6fd46d454d0a56662db751675a6520e1c9f6)

I'm unable to run make successfully because of the following error on my system. I've noticed this in another PR as well and it is due to lack of a commit that was merged in later.
Asked couple questions.

```
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/time.h:127:12: note: 'gmtime_r' declared here
struct tm *gmtime_r(const time_t * __restrict, struct tm *
...

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1605063804)
`"node 0 chain did not reorganize"`
This seems to be the 3rd argument to the `assert_not_equal()` call, i.e. `*args`. Will this piece be kicked in then? `any(thing1 == arg for arg in args)`

I don't get why would we need to search for `main_block_hash` in the above error string.

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2117693875)
@kevkevinpal Did you use a tool to generate the verification script?

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2117698802)
Do we have any information about real-world deployment of PCP vs NAT-PMP? I see PCP dates from 2013, but do we know if it was adopted immediately (and/or, how many older routing devices are in common use)?

RFC 6887 Appendix A explains how one can be compatible with both, though I don't know how much work it would be to implement.

💬 edilmedeiros commented on issue "dumpprivkey error":
(https://github.com/bitcoin/bitcoin/issues/30124#issuecomment-2117722655)
Duplicate of #30129 which has a working solution.

💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2117722792)
Concept ACK making it 10kvb. Assuming we stick with that, then the commit message in the second commit should be updated (https://github.com/bitcoin/bitcoin/commit/1d95eee9ebfa17dee3c9cf60fea1c28a2cf5d8b5).

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2117723501)
i have no idea, also no idea how to get that information; to be honest if we can remotely avoid it, i'd prefer not to implement unnecessary compatibility stuff, this is already a lot to ask people to review as-is.