💬 NowYu commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#issuecomment-3009638607)
https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1876650074

💬 luke-jr commented on pull request "doc: Add fetching single PRs from upstream to productivity.md":
(https://github.com/bitcoin/bitcoin/pull/32783#discussion_r2169785058)
Why the experimental `git switch` instead of `git checkout`?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2169804538)
I don't think this is confusing because of prior context. I can understand the point about newer contributors. I've come across other places in the codebase where I've had to learn the context by combing through IRC, PRs, commit messages, blogs, etc. I think it's similar to adding the information in the commit message rather than including a comment since a user must search for the commit message info.

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3009676579)
See https://github.com/sipa/bitcoin/commits/pr31829:
* Makes a few preparatory behavior changes to make it more simulation-testable (return which announcements are made reconsiderable from `AddChildrenToWorkSet, and tie-break equally-DoSy peers by picking the highest NodeId).
* Add a simulation fuzz test which uses a super dumb vector of (wtxid, nodeid) pairs (in announcement order) to represent the state of the orphanage.
* Adds a `std::set<Wtxid>` of reconsiderable wtxids to prevent having
...

💬 NowYu commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3009713243)
> How does this compare with #32685?

💬 theStack commented on pull request "test: fix catchup loop in outbound eviction functional test":
(https://github.com/bitcoin/bitcoin/pull/32742#issuecomment-3009742101)
@pablomartin4btc:

> not sure if this is an issue but `tip_header.hash` (after this change) will be only used in this file later at [`test_outbound_eviction_blocks_relay_only()`](https://github.com/bitcoin/bitcoin/blob/f27898c8bfe339e2cad09d42e133a05602b1a030/test/functional/p2p_outbound_eviction.py#L235) following the same patter as above, calling `wait_for_getheaders` with `block_hash=tip_header.hash` being `tip_header.hash=None`, should that be fixed too?

Good catch, just verified that t
...

🤔 NowYu reviewed a pull request: "Add read-only mode to sqlite db and use in `bitcoin-wallet`"
(https://github.com/bitcoin/bitcoin/pull/32818#pullrequestreview-2963387333)
Before I get off

💬 willcl-ark commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3009774338)
> How does this compare with #32685?

Ah, I didn't know (or had perhaps forgotten) about that PR. I came via #15608 and did not check what was open. Shame on me :(

I will mark this as draft and take a look over that PR.

📝 willcl-ark converted_to_draft a pull request: "Add read-only mode to sqlite db and use in `bitcoin-wallet`"
(https://github.com/bitcoin/bitcoin/pull/32818)
Currently our `wallet-tool` performs a few "read-only" operations (`info`, `dump`) on sqlite databases. However, currently this involves opening the wallet in RW mode, with the side-effects of modifying the wallet file, and failing to open a "read-only" wallet file.

See #15608 for more context.

Since we have dropped the BDB wallet, this change got a lot simpler; the BDB parser is now custom and only operates in read-only mode anyway.

This change adds a `read_only` bool to `DatabaseOptio
...

💬 luke-jr commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3009784226)
Concept NACK. Stop trying to enable spam.

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169883496)
Is exposing LimitOrphans even worth it vs calling it internally when we add a tx/announcement?

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169961484)
Calling it just internally makes sense to me.

💬 achow101 commented on pull request "wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members":
(https://github.com/bitcoin/bitcoin/pull/32763#issuecomment-3009979920)
> I don't think there's any benefit to refactoring this either?

There is a concrete benefit to developers for knowing exactly what metadata fields are present in CWalletTx. Both `mapValue` and `vOrderForm` are opaque - it's not obvious what data they store just by looking at `CWalletTx`'s definition. Instead, you have to grep around the codebase for `mapValue`, which sometimes is also named `map_value` or `value_map`. Likewise for `vOrderForm`. Sure we can have that comment documenting what i
...

🤔 danielabrozzoni reviewed a pull request: "test: fix catchup loop in outbound eviction functional test"
(https://github.com/bitcoin/bitcoin/pull/32742#pullrequestreview-2963633528)
post merge ACK dd8447f70faf6419b4617da3c1b57098e9cd66a6

Everything looks good, I started reviewing without noticing this was merged already :)

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2170006006)
Agree it can be handled internally. `LimitOrphans` used to allow variable-size limiting (which was also only used for testing) but we're getting rid of that in this PR.

💬 luke-jr commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3010203524)
It makes much more sense to stick to weight for consensus-related matters, and vsize for policy in all cases.

💬 davidgumberg commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3010205780)
> Changing to uint32 needs many changes in FeeFrac (which I would like to avoid touching). In FeeFrac size is defined as an int32_t so using uint32 doesn't give us any "advantage".

Yes, but leaving size as signed in `FeeFrac` means that this PR has to make changes to `CFeeRate` to make its sizes signed as well. `FeeFrac` and `CFeeRate` should both be signed or unsigned, I don't have any context on if/why someone might want signed sizes, but all the `FeeFrac` functions works with signed sizes,
...

🤔 glozow reviewed a pull request: "test: fix an incorrect `feature_fee_estimation.py` subtest"
(https://github.com/bitcoin/bitcoin/pull/32463#pullrequestreview-2963581215)
ACK 9b75cfda4d62a0a3bde402503244dd57e1621a12

💬 glozow commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#discussion_r2169967085)
nit: linking #31384 in the commit message for 9b75cfda4d62a0a3bde402503244dd57e1621a12 makes it easier to see why an extra 4000 is needed

🤔 glozow reviewed a pull request: "mempool: use `FeeFrac` for ancestor/descendant score comparators"
(https://github.com/bitcoin/bitcoin/pull/32799#pullrequestreview-2963925005)
ACK 922adf66ac7420e21ea171d3586ea84554e5d91b
great to get rid of these doubles (🤮)