💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838289018)
will look in follow-up

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470819890)
compiling a list of things to do in follow-up, otherwise keeping as-is

💬 furszy commented on pull request "test: report detailed msg during utf8 response decoding error":
(https://github.com/bitcoin/bitcoin/pull/31251#discussion_r1838292730)
> Out of curiosity: Do you think we should limit printing the data here to a certain length or printing all of it is fine as well? Idk atm if the RPC responses from core have a size limit.

Should check the limit, there shouldn't be any. But, as this is an error message, which shouldn't usually happen, I think that it is better to receive the entire content.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838292789)
will take a look in followup

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838294911)
will look in followup

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838295658)
will look in follow-up

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838296688)
lots of code history blown away I'm sure :)

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838302730)
The same argument should be made for not allowing modified fees too right? There's an argument to be made, but I think it's more expansive than just the RPC.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838305310)
will add in follow-up

🤔 marcofleon reviewed a pull request: "addrman: cap the `max_pct` to not exceed the maximum number of addresses"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2429970052)
Tested ACK 9c5775c331e02dab06c78ecbb58488542d16dda7. Reproduced the crash on master and checked that this fixed it. The checks added to `GetAddr_` look reasonable.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470893774)
addrman test failure for win64, assuming unrelated: https://github.com/bitcoin/bitcoin/pull/30239/checks?check_run_id=32868463159

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838351237)
will take a crack at it in follow-up

💬 fanquake commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2470949674)
> addrman test failure for win64, assuming unrelated:

Yea this is a Wine issue (see #31071 & the linked issues). I restarted the job.

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838145180)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834309312

> Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for `chainStateFlushed`, the `chainStateFlushed` implementation could just do that internally.

Current usages are a little unusual because chainStateFlushed() is used in the wallet both to receive notifications from the node about when to flush, but also internally in the wallet to flush its ow
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838364191)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834365825

> Why is the `role` typed as `UInt32` when the other enums are typed as `Int32`?

Changes this to `Int32` to be consistent. Probably `UInt32` was used because this parameter was added at a later time than all the other code was written. The exact integer type used to represent the enum doesn't actually matter as long as it is wide enough to hold all the enum values, and as long as round-trip static casts from the enum
...

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838369167)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834376651

> Should this include the version as well at this point?

Yes, good catch. Added the version field and added a comment to JSONRPCRequest struct to prevent this type of bug in the future. I think bug this did not matter too much in practice because this capnp struct is only used to forward RPC requests from the node to the wallet and wallet shouldn't respond to requests differently based on JSONRPC version, but it is st
...

🤔 ryanofsky reviewed a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-2429667905)
Updated 968f14fc7a634f65f4399dc042a37d4a39f2c703 -> 34d3e2a6eaaab9de5328c3e64739f1392696c7db ([`pr/ipc-chain.9`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.9) -> [`pr/ipc-chain.10`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-chain.9..pr/ipc-chain.10)) with review suggestions and fixes.

Thanks for the review!

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838152072)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834348410

> Nit: Shouldn't this be called `txid`?

Nice catch, fixed! Might be a good idea to make sure these names are correct, even though they are just documentation and don't affect behavior.

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1838357818)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834362068

> How does this `write` argument here map to the `action` argument in the c++ interface? They seem to have different types.

This is a bug, and it should have been caught by the libmultiprocess library. https://github.com/chaincodelabs/libmultiprocess/pull/120 adds a static_assert to ensure that it would result in a compile error if a specified capnproto type is ever not compatible with a c++ enum type.

🤔 mzumsande reviewed a pull request: "addrman: cap the `max_pct` to not exceed the maximum number of addresses"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2430152067)
Code Review ACK 9c5775c331e02dab06c78ecbb58488542d16dda7