💬 FractalEncrypt commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874362210)
**Difficulty creating transactions with multiple OP_RETURN outputs for testing PR #32406**
I apologize in advance if this is not the right place to ask/post this, I am relatively inexperienced, but trying my best to help.
I'm working on testing PR #32406 and specifically want to create transactions on regtest with multiple large OP_RETURN outputs to stress-test the new functionality. I'm running a build of this PR on Ubuntu (via WSL2).
My goal is to programmatically (ideally via Pytho
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874362210)
**Difficulty creating transactions with multiple OP_RETURN outputs for testing PR #32406**
I apologize in advance if this is not the right place to ask/post this, I am relatively inexperienced, but trying my best to help.
I'm working on testing PR #32406 and specifically want to create transactions on regtest with multiple large OP_RETURN outputs to stress-test the new functionality. I'm running a build of this PR on Ubuntu (via WSL2).
My goal is to programmatically (ideally via Pytho
...
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085633551)
This is an error and should return non-zero exit code.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085633551)
This is an error and should return non-zero exit code.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085634531)
This function is really bad and should've been deprecated, you should use `try_exists` instead.
But also, why do this extra check? Just `read` already does the same thing with less code and less problems.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085634531)
This function is really bad and should've been deprecated, you should use `try_exists` instead.
But also, why do this extra check? Just `read` already does the same thing with less code and less problems.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085633049)
Have you tested that this picks the right one on Windows? That may be a good reason for the attribute.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085633049)
Have you tested that this picks the right one on Windows? That may be a good reason for the attribute.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085643521)
This loads the entire block file into memory. I'm not sure if such memory usage is desirable.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085643521)
This loads the entire block file into memory. I'm not sure if such memory usage is desirable.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085637066)
Why though? After "unxoring" the magic bytes just will be the same. Anyway, might still be a good idea to mask it better.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085637066)
Why though? After "unxoring" the magic bytes just will be the same. Anyway, might still be a good idea to mask it better.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085640823)
Why not reuse the file handle later and perhaps take advantage of `BufReader`?
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085640823)
Why not reuse the file handle later and perhaps take advantage of `BufReader`?
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085638849)
Why not `path.display()`?
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085638849)
Why not `path.display()`?
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085644155)
Yeah, Rust even has `chunks_exact` method for this (and the rest can be xored after the loop). It should also auto-vectorize.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085644155)
Yeah, Rust even has `chunks_exact` method for this (and the rest can be xored after the loop). It should also auto-vectorize.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085644865)
Cool, this is almost atomic. I say almost because you're not calling `sync_data` (because you're not using file handle - I think you should).
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085644865)
Cool, this is almost atomic. I say almost because you're not calling `sync_data` (because you're not using file handle - I think you should).
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085638116)
BTW, this is not atomic. Does it matter? I don't really know. Ideally the tool could continue if it fails but that's not easy.
Still, at least making sure this one doesn't get corrupted may be important?
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2085638116)
BTW, this is not atomic. Does it matter? I don't really know. Ideally the tool could continue if it fails but that's not easy.
Still, at least making sure this one doesn't get corrupted may be important?
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085653496)
Sorry yes you are correct. I misread the usage of `m_cur_cluster` (and then proceeded to also incorrectly fuzz test when trying to confirm my understanding).
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085653496)
Sorry yes you are correct. I misread the usage of `m_cur_cluster` (and then proceeded to also incorrectly fuzz test when trying to confirm my understanding).
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085654357)
Mostly just trying to get an understanding of your thought process on when an Assume() is appropriate. That is good context to know about Assume() usage.
I expected it in the higher-level functions `AddTransaction`, `RemoveTransaction`, `AddDependency`, `SetTransactionFee`, and `CommitStaging` as these all can change linearization of main graph and did indeed find it in these.
I didn't expect it in `ClearChunkData` and `UnlinkRef`, but thought maybe it was there to be confident that we don
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085654357)
Mostly just trying to get an understanding of your thought process on when an Assume() is appropriate. That is good context to know about Assume() usage.
I expected it in the higher-level functions `AddTransaction`, `RemoveTransaction`, `AddDependency`, `SetTransactionFee`, and `CommitStaging` as these all can change linearization of main graph and did indeed find it in these.
I didn't expect it in `ClearChunkData` and `UnlinkRef`, but thought maybe it was there to be confident that we don
...
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874497101)
> The number of possible choices is `tx_count[0] + sims[0].removed.size() + 1`, and this value is stored in the variable `choices`, representing the number of choices.
Interesting, I guess my interpretation is different and don't understand where the +1 comes from. I would have thought the number of choices is simply the tx count plus the removed tx count.
> Sure, that would be correct, but very confusing, I think!
Its funny, I only noticed this because I found the current assignment ve
...
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874497101)
> The number of possible choices is `tx_count[0] + sims[0].removed.size() + 1`, and this value is stored in the variable `choices`, representing the number of choices.
Interesting, I guess my interpretation is different and don't understand where the +1 comes from. I would have thought the number of choices is simply the tx count plus the removed tx count.
> Sure, that would be correct, but very confusing, I think!
Its funny, I only noticed this because I found the current assignment ve
...
💬 murchandamus commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2085698511)
I’m wondering whether there is a bug here. It seems to me that this is assuming that the effective value is only available when SFFO is not being used, but `COutput.effective_value` is set and aggregated whenever the COutputs have a feerate. Does this not explicitly need to handle SFFO? I.e., maybe this needs to be something along the lines of:
```suggestion
CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : av
...
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2085698511)
I’m wondering whether there is a bug here. It seems to me that this is assuming that the effective value is only available when SFFO is not being used, but `COutput.effective_value` is set and aggregated whenever the COutputs have a feerate. Does this not explicitly need to handle SFFO? I.e., maybe this needs to be something along the lines of:
```suggestion
CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : av
...
🤔 murchandamus reviewed a pull request: "wallet: re-activate "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-2834839402)
I’m wondering whether this is correct when SFFO is active.
(https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-2834839402)
I’m wondering whether this is correct when SFFO is active.
💬 murchandamus commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2085699326)
Could this test case be extended by attempting the same transaction with SFFO and showing that it succeeds?
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2085699326)
Could this test case be extended by attempting the same transaction with SFFO and showing that it succeeds?
💬 murchandamus commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2085668477)
Note to reviewers: `preset_inputs.total_amount` will be the sum of the outputs’ effective values if "subtract fee from outputs" is disabled, but will be the nominal amount if SFFO is active. See [`src/wallet/spend.h:164–172`](https://github.com/bitcoin/bitcoin/blob/8cefff322cc6689e50074c93cb9d52412d614d13/src/wallet/spend.h#L164)
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2085668477)
Note to reviewers: `preset_inputs.total_amount` will be the sum of the outputs’ effective values if "subtract fee from outputs" is disabled, but will be the nominal amount if SFFO is active. See [`src/wallet/spend.h:164–172`](https://github.com/bitcoin/bitcoin/blob/8cefff322cc6689e50074c93cb9d52412d614d13/src/wallet/spend.h#L164)
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874657579)
> Interesting, I guess my interpretation is different and don't understand where the +1 comes from. I would have thought the number of choices is simply the tx count plus the removed tx count.
The +1 is from the third option: returning the empty `Ref` object, at the bottom of the function.
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874657579)
> Interesting, I guess my interpretation is different and don't understand where the +1 comes from. I would have thought the number of choices is simply the tx count plus the removed tx count.
The +1 is from the third option: returning the empty `Ref` object, at the bottom of the function.
📝 achow101 opened a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475)
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a boo
...
(https://github.com/bitcoin/bitcoin/pull/32475)
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a boo
...
💬 gmaxwell commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874791860)
utACK while I'm doubtful my comment should carry much weight, I'm wary of the public presentation weight up vacuous opposition, which ought count for nothing, against an absence of positive comments (because the choice is obvious enough that little need be said)... and the most effective thing I can do on that point is to get counted.
I have a slight preference for the closed PR that removes it completely. This is primarily because removing it completely avoids some predictable meta-abuse (e
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874791860)
utACK while I'm doubtful my comment should carry much weight, I'm wary of the public presentation weight up vacuous opposition, which ought count for nothing, against an absence of positive comments (because the choice is obvious enough that little need be said)... and the most effective thing I can do on that point is to get counted.
I have a slight preference for the closed PR that removes it completely. This is primarily because removing it completely avoids some predictable meta-abuse (e
...