💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1837939655)
In commit "coins, refactor: Make AddFlags, SetDirty, SetFresh static" (d5e3bbf440aa948df8159999fd4eb5275c354b93)

It would make sense to rename `self` to `pair` this commit instead of in later commit afb0a3c88691cadc07fc985483f4cae1f486f6be so this code does not have to change twice and so later the change in the later commit can be move-only. Right now the later commit is tedious to review (or at least I don't know an easy way to review it) because the argument is renamed at the same time as
...

👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2429297641)
Code review ACK 9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24. Only small changes since last review like adding an Assume, changing commit messages, renaming self to pair. Thanks for the updates!

I left two suggestions which are not important and don't affect the final code, just meant to make review a little easier for the next reviewer. Feel free to ignore them or save for a later update.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1837922879)
re: https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829401813

> Wouldn't I need to make `AddFlags` static as well in that case?

No that should not be necessary, and wouldn't suggest that. The suggestion is to make the new `SetFresh` and `SetClean` methods static and have them call `self.second.AddFlags` instead of `AddFlags` in this commit so all the new calls to these methods don't need to change again in commit d5e3bbf440aa948df8159999fd4eb5275c354b93.

👍 ryanofsky approved a pull request: "Prune mining interface"
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2429358194)
Code review ACK a67ba64f8f63a0f9f2c86d72cf7240bee7c5388c. Only changes since last review were a tweak to fix a failing test, and a commit message update.

💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1837989979)
Yeah, adding `mempool.m_opts.require_standard` to the if condition would make sense - `acceptnonstdtxn` should turn off all dust checks including this one imo

🤔 ryanofsky reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2429375066)
Updated ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba -> fe39acf88ff552bfc4a502c99774375b91824bb1 ([`pr/tcheck.4`](https://github.com/ryanofsky/bitcoin/commits/pr/tcheck.4) -> [`pr/tcheck.5`](https://github.com/ryanofsky/bitcoin/commits/pr/tcheck.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/tcheck.4..pr/tcheck.5)) with suggested changes. Thanks for the reviews and suggestions!

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837966979)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834472950

> As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.

Thanks, applied suggestion

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837996485)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835766554

> we might as well extract number parsing to a local lambda like you did with the other ones.

This seems reasonable but I"m not sure it's clearer, and it does make the diff bigger replacing the `maybe_num` code that doesn't have to change currently. Happy to apply this suggestion if other reviews think it is a good idea.

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837973744)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834499010

Thanks, added

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837974075)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834504076

Thanks, added

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837983669)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835770704

I can apply the `"#0- +"sv.find` change if others like it, but to me it seems less readable and only a little shorter.

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1837980148)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835770569

Nice suggestion! Applied

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#issuecomment-2470441887)
re: https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2425834191

> Also, not exactly sure why `%n` parity wasn't added

Thanks I hadn't seen #30999, and it seems like that would be a reasonable thing to check for, though I think there is a case for keeping the code as simple as possible and not trying to reproduce tinyformat quirks here. But the reason for not making that change here is I don't think it's related to this PR, and I think it's generally better to make separate c
...

🤔 glozow reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2429428705)
ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

Please lmk what you want to do with the remaining comments, I'm happy to reack or merge with a followup lined up.

💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838062023)
Did you consider benching with the parent in package? I suppose it's about the same.

💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1837998371)
They aren't no-ops. You should be checking `getprioritisedtransactions` instead. You can prio things that aren't in mempool yet, and mempool tracks them so it can be used in validation later.

💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1838074248)
Ah RIP, this is a merge conflict with #30592. That's the CI error.

💬 ryanofsky commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2470510859)
Code review d9cfb2a314caa1cdcae8e47a03dad557ad96e069.

This generally looks good except commit 2d65e6ae0756b87e82dbafd032308e8687ec7941 disallowing `-noconf`. Specifying `-noconf` to disable reading from the default config file path is useful and something that should be supported, even if now it's only working in a very quirky way by treating the datadir as an empty configuration file. I think a better implementation would not look too different though, maybe like:

```diff
--- a/src/commo
...

💬 AndreaDiazCorreia commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2470534984)
I've reviewed and tested the listmempooltransactions RPC implementation. Here's what I did:

- Pulled the branch and rebased it on top of master
- Tested it manually on regtest both with and without transactions in the mempool
- Created a functional test for this RPC. You can find it in my repo at https://github.com/AndreaDiazCorreia/bitcoin/tree/PR29016-listmempooltransactions

The test verifies:
- Basic mempool sequence behavior
- Transaction tracking
- Sequence filtering functionalit
...

💬 vasild commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2470557837)
Right! Looks like for `Discover()` to run `-discover` must be enabled and `-bind` not used. It is tough to explain what a config like `-discover=1 -bind=1.2.3.4:5555` would do. Seems that this is another thing that can be simplified.

Also, this has another flaw: `bind_on_any` will be `false` if `-bind=0.0.0.0:5555` is used, but it should really be `true` :-/