🤔 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
...
💬 Sjors commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171026682)
CI on https://github.com/Sjors/bitcoin/pull/90#issuecomment-3011857912 complains of an unused result.
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171026682)
CI on https://github.com/Sjors/bitcoin/pull/90#issuecomment-3011857912 complains of an unused result.
💬 Sjors commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171082503)
<csignal>
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2171082503)
<csignal>
💬 PeterWrighten commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#discussion_r2171326181)
In this code, you should add more seeds number of platform to pass CI.
(https://github.com/bitcoin/bitcoin/pull/32818#discussion_r2171326181)
In this code, you should add more seeds number of platform to pass CI.
💬 PeterWrighten commented on pull request "Add read-only mode to sqlite db and use in `bitcoin-wallet`":
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3012286236)
> > 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.
Never mind. Your implementation is also great. But seems there is lack of some tests for walletdb and db...
(https://github.com/bitcoin/bitcoin/pull/32818#issuecomment-3012286236)
> > 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.
Never mind. Your implementation is also great. But seems there is lack of some tests for walletdb and db...
💬 naiyoma commented on pull request "wallet: have external signer use PSBT error code EXTERNAL_SIGNER_NOT_FOUND":
(https://github.com/bitcoin/bitcoin/pull/32682#issuecomment-3012316854)
Post Merge TAck 9dfc61d
I tested this by setting up HWI and using Trezor as my external signer
I noticed while reviewing this that some tests have been commented out —> https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L81 and also
https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L171
Perhaps a good follow-up would be to work on as well, ?
(https://github.com/bitcoin/bitcoin/pull/32682#issuecomment-3012316854)
Post Merge TAck 9dfc61d
I tested this by setting up HWI and using Trezor as my external signer
I noticed while reviewing this that some tests have been commented out —> https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L81 and also
https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L171
Perhaps a good follow-up would be to work on as well, ?