🤔 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 (🤮)

💬 glozow commented on pull request "mempool: use `FeeFrac` for ancestor/descendant score comparators":
(https://github.com/bitcoin/bitcoin/pull/32799#discussion_r2170167694)
nit: this (and the others) could be const

💬 davidgumberg commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2170184290)
> Also, it looks like `LockCoin` could return `false`, but the in-memory cache is set.

Agreed, although it makes sense to set in-memory values before persisting in general, it would not make sense here since the user would see an error indicating that the coin had not been locked, even though it is locked in memory.

🤔 pablomartin4btc reviewed a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2964023741)
tACK 6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97

I was able to reproduce the issue on `master`. As a detail, the CLI outputs "_Unable to make a backup of your wallet_" when the backup fails—might be worth mentioning that in the PR description.

This branch fixes the problem as expected (checked both relatives `.../../` and `a/b/c`).

Note that legacy wallet creation is no longer possible on `master`, so it might be helpful to update the reproduction steps. One option could be to use a previ
...

🤔 pablomartin4btc reviewed a pull request: "wallet: Always set descriptor cache upgraded flag for new wallets"
(https://github.com/bitcoin/bitcoin/pull/32597#pullrequestreview-2964165780)
post-merge tACK 47237cd1938058b29fdec242c3a37611e255fda0

> Newly created wallets will always have an upgraded descriptor cache, so set those.

Yes, because the other scenario when the flag is set is when (~after) the wallet is loaded (and descriptor cache is upgraded).

> Also, to verify this behavior, add a new flags field to getwalletinfo and check that in the functional tests.

Checked.

💬 Eunovo commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3011530424)
> 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".

It's difficult for me to give an opinion here because I'm not sure why `FeeFrac` was designed with signed sizes in the first place.
I see that `FeeFrac::EvaluateFee` does have an `Assume(at_size >= 0)` check, I think you should rely on this check when you revert instead of returning `-1` (as was done in https://gi
...

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3011745929)
> though I think the last two commits can be squashed
Done!