💬 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
...
💬 bigshiny90 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874819011)
> @BootsStribling
>
> > This PR achieves none of the stated benefits of #32359 which were:
> >
> > * reduction of harm through the use of OP_RETURN rather than witness storage
> >
> > The ultimate result of this particular PR will be:
> >
> > * continued use of witness data field for arbitrary data incentivized by lower cost from witness discount
>
> The harm reduction argument is that OP_RETURN use allows L2 users an alternative to stuffing arbitrary data into non-provably unspen
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874819011)
> @BootsStribling
>
> > This PR achieves none of the stated benefits of #32359 which were:
> >
> > * reduction of harm through the use of OP_RETURN rather than witness storage
> >
> > The ultimate result of this particular PR will be:
> >
> > * continued use of witness data field for arbitrary data incentivized by lower cost from witness discount
>
> The harm reduction argument is that OP_RETURN use allows L2 users an alternative to stuffing arbitrary data into non-provably unspen
...
🤔 Kixunil reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2835095098)
Concept ACK, the code looks sensible from brief look, though it's unclear what the intention of the parameter was.
I'd also like to ask the opposition to address the numerous arguments about the limit driving centralization and causing long-term storage problems as well as causing the size of the chain to **increase** in the **mailinglist**. I haven't seen a single person attempting to do that yet.
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2835095098)
Concept ACK, the code looks sensible from brief look, though it's unclear what the intention of the parameter was.
I'd also like to ask the opposition to address the numerous arguments about the limit driving centralization and causing long-term storage problems as well as causing the size of the chain to **increase** in the **mailinglist**. I haven't seen a single person attempting to do that yet.
💬 Kixunil commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2085799758)
This doesn't account for the length of serialization of value (8B). So after this change if one wants to keep the 80B limit, the transaction can actually increase by a lot more (80*9 if I count correctly). While I find the arguments for keeping the limit unconvincing this seems to not follow the intention. (But it is true that the amount cannot contain *arbitrary* data.)
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2085799758)
This doesn't account for the length of serialization of value (8B). So after this change if one wants to keep the 80B limit, the transaction can actually increase by a lot more (80*9 if I count correctly). While I find the arguments for keeping the limit unconvincing this seems to not follow the intention. (But it is true that the amount cannot contain *arbitrary* data.)
💬 monlovesmango commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874964921)
reACK 8673e8f01917b48a5f5476792f759f44ea49d5a5
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874964921)
reACK 8673e8f01917b48a5f5476792f759f44ea49d5a5
💬 1440000bytes commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2875133307)
@FractalEncrypt you can use python to create a transaction with multiple OP_RETURN, `fundrawtransaction` to add inputs, `signrawtransactionwithwallet` to sign and `sendrawtransaction` to broadcast it later. It will be listed in the output for `getrawmempool` once broadcasted.
```python
from bitcoin.core import CTransaction, CTxOut, CScript, b2x
from bitcoin.core.script import OP_RETURN
inputs = []
outputs = [
CTxOut(0, CScript([OP_RETURN, b'deadbeef'])),
CTxOut(0, CScript([OP
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2875133307)
@FractalEncrypt you can use python to create a transaction with multiple OP_RETURN, `fundrawtransaction` to add inputs, `signrawtransactionwithwallet` to sign and `sendrawtransaction` to broadcast it later. It will be listed in the output for `getrawmempool` once broadcasted.
```python
from bitcoin.core import CTransaction, CTxOut, CScript, b2x
from bitcoin.core.script import OP_RETURN
inputs = []
outputs = [
CTxOut(0, CScript([OP_RETURN, b'deadbeef'])),
CTxOut(0, CScript([OP
...
💬 delta1 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2875159636)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2875159636)
Concept ACK
💬 maflcko commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2875226336)
Seems fine. I tested not this pull, but `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-std=c++23 -O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG'` and found that some benchmarks are slower by 10x (`--filter=EvictionProtection3Networks100Candidates`), however this should be fine, as it can be disabled. Funnily, some are even *faster*, like (`--
...
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2875226336)
Seems fine. I tested not this pull, but `rm -rf ./bld-cmake && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-std=c++23 -O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG'` and found that some benchmarks are slower by 10x (`--filter=EvictionProtection3Networks100Candidates`), however this should be fine, as it can be disabled. Funnily, some are even *faster*, like (`--
...
📝 maflcko opened a pull request: "refactor: Remove unused HaveKey and HaveWatchOnly"
(https://github.com/bitcoin/bitcoin/pull/32476)
removes some dead code
(https://github.com/bitcoin/bitcoin/pull/32476)
removes some dead code
💬 maflcko commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2875411238)
The final possible bdb removals can be found via `git grep -i 'legacy wallet' src/bitcoin-wallet.cpp src/wallet src/script/`.
Some may be fine to keep as historic comment and others are not a clean removal, some may need to replaced by a Assert/throw or `util::Error`.
In any case, if the untested code isn't removed, it should be tested:
* https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/db.cpp.gcov.html
* https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/dump.cpp.
...
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2875411238)
The final possible bdb removals can be found via `git grep -i 'legacy wallet' src/bitcoin-wallet.cpp src/wallet src/script/`.
Some may be fine to keep as historic comment and others are not a clean removal, some may need to replaced by a Assert/throw or `util::Error`.
In any case, if the untested code isn't removed, it should be tested:
* https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/db.cpp.gcov.html
* https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/dump.cpp.
...
💬 maflcko commented on pull request "refactor: Remove unused HaveKey and HaveWatchOnly":
(https://github.com/bitcoin/bitcoin/pull/32476#issuecomment-2875412918)
(follow-up to https://github.com/bitcoin/bitcoin/pull/32438)
(https://github.com/bitcoin/bitcoin/pull/32476#issuecomment-2875412918)
(follow-up to https://github.com/bitcoin/bitcoin/pull/32438)