💬 maflcko commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049916511)
Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed.
That is, old wallets will (obviously) continue to write their `CLIENT_VERSION` to the `version` record. Also, any new wallet will continue to write a value of the current `CLIENT_VERSION` to it, just like on the current development branch (this is unchanged as well). The only change is tha
...
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049916511)
Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed.
That is, old wallets will (obviously) continue to write their `CLIENT_VERSION` to the `version` record. Also, any new wallet will continue to write a value of the current `CLIENT_VERSION` to it, just like on the current development branch (this is unchanged as well). The only change is tha
...
💬 achow101 commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049933892)
> Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed.
Sorry, I was responding to @ryanofsky's comment at the same time since they were very similar.
I think that may be workable.
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3049933892)
> Maybe I am missing something obvious, but my suggestion was not to introduce a new field/record. It was to re-use the existing field (and value) and manually handle the value going forward only when needed.
Sorry, I was responding to @ryanofsky's comment at the same time since they were very similar.
I think that may be workable.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193207335)
Done. I didn't really know the best name for this initially. I like `TxidOrWtxid` a bit better.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193207335)
Done. I didn't really know the best name for this initially. I like `TxidOrWtxid` a bit better.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193209465)
Took this suggestion 🤝
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193209465)
Took this suggestion 🤝
💬 achow101 commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3049952776)
Reordered as suggested
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3049952776)
Reordered as suggested
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193216368)
I agree that there isn't a need for branching on wtxid/txid and that having the comment there is enough. Just to be sure, @glozow if you prefer to keep the branching, let me know and I can revert.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193216368)
I agree that there isn't a need for branching on wtxid/txid and that having the comment there is enough. Just to be sure, @glozow if you prefer to keep the branching, let me know and I can revert.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193217740)
Good catch, same for `ForgetTxHash`.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193217740)
Good catch, same for `ForgetTxHash`.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193219736)
Good looks
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2193219736)
Good looks
💬 luke-jr commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3050037676)
crACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3050037676)
crACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193283389)
Good point, I will add a test for this.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193283389)
Good point, I will add a test for this.
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193284376)
Yes, I will look into implementing the separate size limit.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193284376)
Yes, I will look into implementing the separate size limit.
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193285093)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193285093)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193289150)
I'm not sure if that name would be accurate, because this boolean is set to `true` even when we are trying to create a v3 transaction, in which case we are technically not excluding version 3.
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2193289150)
I'm not sure if that name would be accurate, because this boolean is set to `true` even when we are trying to create a v3 transaction, in which case we are technically not excluding version 3.
💬 mzumsande commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#issuecomment-3050069477)
> maybe I'm missing something but even without the calc_flags_and_header=false flag, this means that [the condition](https://github.com/bitcoin/bitcoin/blob/a8bff38236ac988f82dcfa85438946cfe0d3afe3/src/validation.cpp#L2074) cannot hit true and RecalculateBestHeader() in InvalidChainFound() cannot be called right?
Yes, that's true, good point! To phrase it in my words, the condition that checks whether `m_best_header` needs update will not be fulfilled, because `m_best_header` was already succ
...
(https://github.com/bitcoin/bitcoin/pull/32843#issuecomment-3050069477)
> maybe I'm missing something but even without the calc_flags_and_header=false flag, this means that [the condition](https://github.com/bitcoin/bitcoin/blob/a8bff38236ac988f82dcfa85438946cfe0d3afe3/src/validation.cpp#L2074) cannot hit true and RecalculateBestHeader() in InvalidChainFound() cannot be called right?
Yes, that's true, good point! To phrase it in my words, the condition that checks whether `m_best_header` needs update will not be fulfilled, because `m_best_header` was already succ
...
🤔 luke-jr requested changes to a pull request: "Cache m_cached_finished_ibd where SetTip is called."
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2998927241)
Concept ACK, but I'm not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it's not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-2998927241)
Concept ACK, but I'm not convinced this implementation is safe as-is. If we want to maintain the current behaviour, it's not sufficient to update only when the tip changes. We also need to re-check when importing/reindexing completes, and schedule an update timer if max_tip_age is the final cause of not exiting IBD.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353160)
I do agree, just seemed more "idiomatic" to handle the wallet_name variable with the `has_value()`, but yeah, not very practical in the end. I've removed the `MaybeArg` extension commit making the `wallet_name` to hold a pointer in the `GetWalletNameFromJSONRPCRequest` overload.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353160)
I do agree, just seemed more "idiomatic" to handle the wallet_name variable with the `has_value()`, but yeah, not very practical in the end. I've removed the `MaybeArg` extension commit making the `wallet_name` to hold a pointer in the `GetWalletNameFromJSONRPCRequest` overload.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353278)
As in [previous](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500) comment, already removed the commit containing also this change. I've analysed the [discussion](https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286815104) around the string copy in the original implementation of `MaybeArg` and even if we could go around it and "fix"/ adding it for the string specific case/ condition (~`return std::optional<std::string_view>{params[i].get_str_ref()}`?) perhaps makes
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353278)
As in [previous](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500) comment, already removed the commit containing also this change. I've analysed the [discussion](https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286815104) around the string copy in the original implementation of `MaybeArg` and even if we could go around it and "fix"/ adding it for the string specific case/ condition (~`return std::optional<std::string_view>{params[i].get_str_ref()}`?) perhaps makes
...
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353506)
True, not really needed there, but resolved the `getdescriptoractivity` RPC issue using `std::ranges::all_of)` instead, as you suggested.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2193353506)
True, not really needed there, but resolved the `getdescriptoractivity` RPC issue using `std::ranges::all_of)` instead, as you suggested.
💬 luke-jr commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2193383756)
The null wtxid is already added at the top outside the loop.
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2193383756)
The null wtxid is already added at the top outside the loop.
💬 luke-jr commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2193403333)
Yes, that's what I forgot.
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2193403333)
Yes, that's what I forgot.