💬 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.
🤔 glozow reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2999048711)
lightly reviewed ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2999048711)
lightly reviewed ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9
💬 glozow commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2193405519)
Maybe worth adding that "All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed." Somebody who sees a [*] may wonder what that means, and hopefully find that in the release notes.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2193405519)
Maybe worth adding that "All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed." Somebody who sees a [*] may wonder what that means, and hopefully find that in the release notes.
💬 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-3050272556)
I've updated the PR to do a combination of @maflcko's and @ryanofsky's suggestions.
The new approach decouples `version` from CLIENT_VERSION. It also writes out a set of client specific feature flags into two new records: `lastopenedfeatures` and `lastdecryptedfeatures`. However, these feature flags are separate from the existing wallet feature flags.
I believe this preserves the behavior of detecting downgrades down to v24.0, while also giving us the flexibility of a new field and feature
...
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3050272556)
I've updated the PR to do a combination of @maflcko's and @ryanofsky's suggestions.
The new approach decouples `version` from CLIENT_VERSION. It also writes out a set of client specific feature flags into two new records: `lastopenedfeatures` and `lastdecryptedfeatures`. However, these feature flags are separate from the existing wallet feature flags.
I believe this preserves the behavior of detecting downgrades down to v24.0, while also giving us the flexibility of a new field and feature
...
💬 luke-jr commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2193445862)
Sorry, mixed that up with `have_witness_proof`.
This is to explicitly specify that the proof is proving the generation tx itself, not just using it to prove the witness root.
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2193445862)
Sorry, mixed that up with `have_witness_proof`.
This is to explicitly specify that the proof is proving the generation tx itself, not just using it to prove the witness root.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3050401651)
<ins>_Updates_</ins>
- Addressed @maflcko's feedback:
- [dropped](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500) `MaybeArg` extension commit, since a pointer already can hold the null state needed in the validations within the `GetWalletNameFromJSONRPCRequest` overload;
- removed previously added `AreParamsNullOrEmpty` and replaced it with std::ranges::all_of in the `getdescriptoractivity` RPC fix instead, as [suggested](https://github.com/bitcoin/bitcoin/pull/32845
...
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3050401651)
<ins>_Updates_</ins>
- Addressed @maflcko's feedback:
- [dropped](https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500) `MaybeArg` extension commit, since a pointer already can hold the null state needed in the validations within the `GetWalletNameFromJSONRPCRequest` overload;
- removed previously added `AreParamsNullOrEmpty` and replaced it with std::ranges::all_of in the `getdescriptoractivity` RPC fix instead, as [suggested](https://github.com/bitcoin/bitcoin/pull/32845
...
💬 mzumsande commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2193512334)
FYI test fails because of some windows path-printing isues.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2193512334)
FYI test fails because of some windows path-printing isues.