💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1663941035)
Thank you for the re-review @MarcoFalke,
Updated 8c4481ed3713931247e4cedcb5733d3598050eb7 -> 8c4481ed3713931247e4cedcb5733d3598050eb7 ([cleaveLeveldbHeaders_2](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_2) -> [cleaveLeveldbHeaders_3](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_2..cleaveLeveldbHeaders_3))
* Addressed @MarcoFalke's [comment](https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1663941035)
Thank you for the re-review @MarcoFalke,
Updated 8c4481ed3713931247e4cedcb5733d3598050eb7 -> 8c4481ed3713931247e4cedcb5733d3598050eb7 ([cleaveLeveldbHeaders_2](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_2) -> [cleaveLeveldbHeaders_3](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_2..cleaveLeveldbHeaders_3))
* Addressed @MarcoFalke's [comment](https://github.com/bitcoi
...
👍 dergoegge approved a pull request: "fuzz: addrman, avoid `ConsumeDeserializable` when possible"
(https://github.com/bitcoin/bitcoin/pull/27918#pullrequestreview-1561046861)
Code review ACK 025fda0a76893d09d19ec9a6c0ba86ad11c466f7
(https://github.com/bitcoin/bitcoin/pull/27918#pullrequestreview-1561046861)
Code review ACK 025fda0a76893d09d19ec9a6c0ba86ad11c466f7
💬 sp-marcel-hernandez commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663944157)
> FYI if merged, this pull-req would allow you to continue to limit or block data-carrying OP_Return outputs by setting `-datacarriersize=n` or `-datacarriersize=0` respectively.
I understand that, it's a matter of trust. What I mean is that if the Bitcoin developers end up sanctioning such an obvious sabotage on the system I wouldn't trust them as a group to continue developing the reference implementation.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663944157)
> FYI if merged, this pull-req would allow you to continue to limit or block data-carrying OP_Return outputs by setting `-datacarriersize=n` or `-datacarriersize=0` respectively.
I understand that, it's a matter of trust. What I mean is that if the Bitcoin developers end up sanctioning such an obvious sabotage on the system I wouldn't trust them as a group to continue developing the reference implementation.
💬 1ma commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663948260)
> FYI if merged, this pull-req would allow you to continue to limit or block data-carrying OP_Return outputs by setting `-datacarriersize=n` or `-datacarriersize=0` respectively. Of course, if a non-trivial minority of nodes and miners choose to relay and mine such transactions, you blocking them will have no impact as they will be propagated and mined anyway.
I understand that, it's a matter of trust. What I mean is that if the Bitcoin developers end up sanctioning such an obvious sabotage o
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663948260)
> FYI if merged, this pull-req would allow you to continue to limit or block data-carrying OP_Return outputs by setting `-datacarriersize=n` or `-datacarriersize=0` respectively. Of course, if a non-trivial minority of nodes and miners choose to relay and mine such transactions, you blocking them will have no impact as they will be propagated and mined anyway.
I understand that, it's a matter of trust. What I mean is that if the Bitcoin developers end up sanctioning such an obvious sabotage o
...
💬 ismaelsadeeq commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1283174480)
Okay, I was asking because I see a test for that in `mempool_compatibility.py`, probably that's why CI is failing, `mempool.dat` future versions are to be backwards incompatible then we can remove the `mempool_compatibility.py` test along with this?
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1283174480)
Okay, I was asking because I see a test for that in `mempool_compatibility.py`, probably that's why CI is failing, `mempool.dat` future versions are to be backwards incompatible then we can remove the `mempool_compatibility.py` test along with this?
💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1283175570)
1) I don't think the redundancy is warranted going forwards.
2) Re: backwards compatibility, I expect that only a very small number of people have set `-datacarrier=0`, so I'm fine with those few people having to set `-datacarriersize=0`.
Though if there is strong demand, I could make `-datacarrier=0` act as an override on `-datacarriersize`, forcing the latter to zero.
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1283175570)
1) I don't think the redundancy is warranted going forwards.
2) Re: backwards compatibility, I expect that only a very small number of people have set `-datacarrier=0`, so I'm fine with those few people having to set `-datacarriersize=0`.
Though if there is strong demand, I could make `-datacarrier=0` act as an override on `-datacarriersize`, forcing the latter to zero.
💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663953595)
> I understand that, it's a matter of trust. What I mean is that if the Bitcoin developers end up sanctioning such an obvious sabotage on the system I won't trust them as a group to continue developing the reference implementation.
We still "sanction" even more "obvious sabotage" by the fact that bare multisig outputs are relayed by default. If you have a complaint, I'd suggest you start there rather than on OP_Return, a mechanism designed specifically for low impact and vigorously discussed
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663953595)
> I understand that, it's a matter of trust. What I mean is that if the Bitcoin developers end up sanctioning such an obvious sabotage on the system I won't trust them as a group to continue developing the reference implementation.
We still "sanction" even more "obvious sabotage" by the fact that bare multisig outputs are relayed by default. If you have a complaint, I'd suggest you start there rather than on OP_Return, a mechanism designed specifically for low impact and vigorously discussed
...
💬 MarcoFalke commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1283182371)
I think you also changed `-datacarriersize` to always be `+1`, so setting it to zero has a different meaning.
The cost seems low to keep the meaning and features the same, so personally I think it is fine to keep them the same.
In any case, release notes are needed
* for all removed features,
* for all changed meanings,
* for all lifted restrictions, and
* for all default value changes.
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1283182371)
I think you also changed `-datacarriersize` to always be `+1`, so setting it to zero has a different meaning.
The cost seems low to keep the meaning and features the same, so personally I think it is fine to keep them the same.
In any case, release notes are needed
* for all removed features,
* for all changed meanings,
* for all lifted restrictions, and
* for all default value changes.
💬 eriknylund commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1663965600)
@Sjors I've had a look at adding the tests mentioned in the issue by extending the tests in `wallet_taproot.py`. I'll open a PR - happy to get feedback on it.
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1663965600)
@Sjors I've had a look at adding the tests mentioned in the issue by extending the tests in `wallet_taproot.py`. I'll open a PR - happy to get feedback on it.
💬 MarcoFalke commented on pull request "build: Bump minimum supported Clang to clang-13":
(https://github.com/bitcoin/bitcoin/pull/28210#issuecomment-1663968993)
This fails on `macOS 11.0`, according to CI. So I guess this can be closed for now.
(https://github.com/bitcoin/bitcoin/pull/28210#issuecomment-1663968993)
This fails on `macOS 11.0`, according to CI. So I guess this can be closed for now.
💬 1ma commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663982564)
Yeah, I know all about it. To this day Bitcoin Core still relays bare multisig TXs by default because back when it was considered making them non-standard Mike Hearn weighted in with a meaningless objection: https://github.com/bitcoin/bitcoin/pull/5231
Now that you mention it maybe it would be a good time to reconsider that again, though that belongs on another PR.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1663982564)
Yeah, I know all about it. To this day Bitcoin Core still relays bare multisig TXs by default because back when it was considered making them non-standard Mike Hearn weighted in with a meaningless objection: https://github.com/bitcoin/bitcoin/pull/5231
Now that you mention it maybe it would be a good time to reconsider that again, though that belongs on another PR.
💬 dergoegge commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283180969)
style nit:
```suggestion
/*nDerivationMethod=*/nDerivationMethod);
```
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283180969)
style nit:
```suggestion
/*nDerivationMethod=*/nDerivationMethod);
```
💬 dergoegge commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283205607)
I don't quite understand why the CI isn't failing here. You're using the new macro from #28065 but you haven't rebased past that PR, so this shouldn't work.
These are the errors that i get locally:
```
wallet/test/fuzz/crypter.cpp:22:22: error: too many arguments provided to function-like macro invocation
FUZZ_TARGET(crypter, .init = initialize_crypter)
^
./test/fuzz/fuzz.h:31:9: note: macro 'FUZZ_TARGET' defined here
#define FUZZ_TARGET(name) \
^
wallet/t
...
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283205607)
I don't quite understand why the CI isn't failing here. You're using the new macro from #28065 but you haven't rebased past that PR, so this shouldn't work.
These are the errors that i get locally:
```
wallet/test/fuzz/crypter.cpp:22:22: error: too many arguments provided to function-like macro invocation
FUZZ_TARGET(crypter, .init = initialize_crypter)
^
./test/fuzz/fuzz.h:31:9: note: macro 'FUZZ_TARGET' defined here
#define FUZZ_TARGET(name) \
^
wallet/t
...
💬 dergoegge commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283174429)
style nit, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
```suggestion
const unsigned int derivation_method = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();
```
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283174429)
style nit, see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
```suggestion
const unsigned int derivation_method = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<unsigned int>();
```
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283210531)
CI does a "rebase" before each run. Generally this detects more issues (silent merge conflicts), and I think this is the first case that CI missed a compile failure of this kind. (Generally it is assumed that developers at a minimum try to compile their changes locally, so it would be odd to have a dedicated CI check for that, but maybe there should be?)
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283210531)
CI does a "rebase" before each run. Generally this detects more issues (silent merge conflicts), and I think this is the first case that CI missed a compile failure of this kind. (Generally it is assumed that developers at a minimum try to compile their changes locally, so it would be odd to have a dedicated CI check for that, but maybe there should be?)
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283212549)
Now that we are moving to persistent workers, we can even compile all commits in a pull request. Maybe as a separate task on a runner where it doesn't matter when the CI run takes a day or two?
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283212549)
Now that we are moving to persistent workers, we can even compile all commits in a pull request. Maybe as a separate task on a runner where it doesn't matter when the CI run takes a day or two?
💬 petertodd commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1283214778)
> I think you also changed `-datacarriersize` to always be `+1`, so setting it to zero has a different meaning.
>
> The cost seems low to keep the meaning and features the same, so personally I think it is fine to keep them the same.
The difference in meaning here is what I would consider to be a bug fix. Previously these options also applied to non-data-carrying OP_Return outputs, which doesn't make any sense. Anyone actually using `-datacarriersize` would just increment the amount accepte
...
(https://github.com/bitcoin/bitcoin/pull/28130#discussion_r1283214778)
> I think you also changed `-datacarriersize` to always be `+1`, so setting it to zero has a different meaning.
>
> The cost seems low to keep the meaning and features the same, so personally I think it is fine to keep them the same.
The difference in meaning here is what I would consider to be a bug fix. Previously these options also applied to non-data-carrying OP_Return outputs, which doesn't make any sense. Anyone actually using `-datacarriersize` would just increment the amount accepte
...
💬 dergoegge commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283216754)
> Now that we are moving to persistent workers, we can even compile all commits in a pull request.
Sounds good but would that really take one or two days? With caching this should only take marginally longer, no?
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283216754)
> Now that we are moving to persistent workers, we can even compile all commits in a pull request.
Sounds good but would that really take one or two days? With caching this should only take marginally longer, no?
💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283223804)
```suggestion
void CDBBatch::WriteImpl(Span<const std::byte> ssKey, CDataStream& ssValue)
```
Span is cheap to copy, so there is no need to add `const&`. Also, it would be better to do the renames in a separate commit. (Maybe at the end for all touched lines in this pull, or not at all). Otherwise, this breaks review options such as `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283223804)
```suggestion
void CDBBatch::WriteImpl(Span<const std::byte> ssKey, CDataStream& ssValue)
```
Span is cheap to copy, so there is no need to add `const&`. Also, it would be better to do the renames in a separate commit. (Maybe at the end for all touched lines in this pull, or not at all). Otherwise, this breaks review options such as `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283227847)
I am not sure how much CPU we want to spend on this. It should only ever find an issue once every couple of months. And there could be quite a backlog easily. For example, 3 devs could each force push a 15 commit pull that touches `serialize.h` at the same time. So the cache would be useless and the CI would have to do 45 builds from scratch.
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1283227847)
I am not sure how much CPU we want to spend on this. It should only ever find an issue once every couple of months. And there could be quite a backlog easily. For example, 3 devs could each force push a 15 commit pull that touches `serialize.h` at the same time. So the cache would be useless and the CI would have to do 45 builds from scratch.