💬 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
...
(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 :)
(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.
(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.
(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,
...
(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
(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
(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 (🤮)
(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
(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.
(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
...
(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.
(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
...
(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!
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3011745929)
> though I think the last two commits can be squashed
Done!
💬 Eunovo commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3011790118)
@rkrux I think the `include_watchonly` param is also outdated and should be removed. Watch-only descriptor wallet balances are marked as spendable, so `include_watchonly` no longer serves any purpose.
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3011790118)
@rkrux I think the `include_watchonly` param is also outdated and should be removed. Watch-only descriptor wallet balances are marked as spendable, so `include_watchonly` no longer serves any purpose.
💬 Eunovo commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#discussion_r2170890355)
https://github.com/bitcoin/bitcoin/pull/32761/commits/220074ef094866386211db4d647d5ff1c93ba54e: Consider "Note: For watch-only wallets, this returns the balance of monitored addresses, but funds cannot actually be spent"
(https://github.com/bitcoin/bitcoin/pull/32761#discussion_r2170890355)
https://github.com/bitcoin/bitcoin/pull/32761/commits/220074ef094866386211db4d647d5ff1c93ba54e: Consider "Note: For watch-only wallets, this returns the balance of monitored addresses, but funds cannot actually be spent"
💬 maflcko commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3011840067)
> @rkrux I think the `include_watchonly` param is also outdated and should be removed. Watch-only descriptor wallet balances are marked as spendable, so `include_watchonly` no longer serves any purpose.
See https://github.com/bitcoin/bitcoin/pull/32618
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3011840067)
> @rkrux I think the `include_watchonly` param is also outdated and should be removed. Watch-only descriptor wallet balances are marked as spendable, so `include_watchonly` no longer serves any purpose.
See https://github.com/bitcoin/bitcoin/pull/32618
💬 maflcko commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3011875412)
Same here:
```
building /gnu/store/z3s5wmdqmbznsh7i1wvs0y9lw7zmqw1w-gnutls-3.7.7.tar.xz.drv...
Starting download of /gnu/store/sziaihhn2rs2p95gfn83yi78b9dlj2qi-gnutls-3.7.7.tar.xz
From http://artfiles.org/gnupg.org/gnutls/v3.7/gnutls-3.7.7.tar.xz...
download failed "http://artfiles.org/gnupg.org/gnutls/v3.7/gnutls-3.7.7.tar.xz" 404 "Not Found"
Starting download of /gnu/store/sziaihhn2rs2p95gfn83yi78b9dlj2qi-gnutls-3.7.7.tar.xz
From http://www.crysys.hu/gnutls/v3.7/gnutls-3.7.7.tar.xz...
foll
...
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3011875412)
Same here:
```
building /gnu/store/z3s5wmdqmbznsh7i1wvs0y9lw7zmqw1w-gnutls-3.7.7.tar.xz.drv...
Starting download of /gnu/store/sziaihhn2rs2p95gfn83yi78b9dlj2qi-gnutls-3.7.7.tar.xz
From http://artfiles.org/gnupg.org/gnutls/v3.7/gnutls-3.7.7.tar.xz...
download failed "http://artfiles.org/gnupg.org/gnutls/v3.7/gnutls-3.7.7.tar.xz" 404 "Not Found"
Starting download of /gnu/store/sziaihhn2rs2p95gfn83yi78b9dlj2qi-gnutls-3.7.7.tar.xz
From http://www.crysys.hu/gnutls/v3.7/gnutls-3.7.7.tar.xz...
foll
...
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3011908517)
> It makes much more sense to stick to weight for consensus-related matters, and vsize for policy in all cases.
Sorry, I didn't quite fully understand the comment (do you mean sigopsize should return only `int64_t sigopWeight = entry->GetSigOpCost() * nBytesPerSigOp;`) can you elaborate more on it, and what you are suggesting. Thanks.
(https://github.com/bitcoin/bitcoin/pull/32800#issuecomment-3011908517)
> It makes much more sense to stick to weight for consensus-related matters, and vsize for policy in all cases.
Sorry, I didn't quite fully understand the comment (do you mean sigopsize should return only `int64_t sigopWeight = entry->GetSigOpCost() * nBytesPerSigOp;`) can you elaborate more on it, and what you are suggesting. Thanks.
💬 yuvicc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3011987044)
lgtm re-ACK be8f3053a7ad526823f32f1a70847b9c06c4c54b
Before e87430ed5fad6f9d90c1e7d256b9b8276b936c24
| ns/leaf | leaf/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 53.83 | 18,577,129.91 | 0.1% | 1.10 | `MerkleRoot`
| 53.62 | 18,648,858.81 | 0.1% | 1.10 | `MerkleRoot`
| 53.70 | 18,621,594.40 | 0.1% | 1.10 | `Mer
...
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3011987044)
lgtm re-ACK be8f3053a7ad526823f32f1a70847b9c06c4c54b
Before e87430ed5fad6f9d90c1e7d256b9b8276b936c24
| ns/leaf | leaf/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 53.83 | 18,577,129.91 | 0.1% | 1.10 | `MerkleRoot`
| 53.62 | 18,648,858.81 | 0.1% | 1.10 | `MerkleRoot`
| 53.70 | 18,621,594.40 | 0.1% | 1.10 | `Mer
...