💬 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.
💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283217052)
didn't realize `preciousblock` also reconsidered the block!
was thinking `reconsiderblock`
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283217052)
didn't realize `preciousblock` also reconsidered the block!
was thinking `reconsiderblock`
💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283247917)
Ok I feel like I understand the test, but I still don't know why it's important the orphan is fake so I'm likely missing something
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283247917)
Ok I feel like I understand the test, but I still don't know why it's important the orphan is fake so I'm likely missing something
💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283215687)
much clearer, thanks
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283215687)
much clearer, thanks
💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283241941)
```suggestion
peer_normal.send_and_ping(msg_tx(tx_parent_arrives["tx"]))
assert tx_parent_arrives["txid"] in node.getrawmempool()
```
my brain for some reason was thinking it was in the orphan pool as well
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283241941)
```suggestion
peer_normal.send_and_ping(msg_tx(tx_parent_arrives["tx"]))
assert tx_parent_arrives["txid"] in node.getrawmempool()
```
my brain for some reason was thinking it was in the orphan pool as well
📝 glozow converted_to_draft a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031)
Depends on #28199, please review that first.
This is milestone 1 of package relay p2p changes. See #27463 for full project tracking.
Please see #27742 for how this PR fits into the big picture. I strongly suggest that reviewers look at that PR first to decide if they are comfortable with the overall approach.
This PR is mainly refactors, with a few behavior changes and improvements:
- Introduces `TxPackageTracker`, which starts out as a wrapper for the `TxOrphanage`. This will be the m
...
(https://github.com/bitcoin/bitcoin/pull/28031)
Depends on #28199, please review that first.
This is milestone 1 of package relay p2p changes. See #27463 for full project tracking.
Please see #27742 for how this PR fits into the big picture. I strongly suggest that reviewers look at that PR first to decide if they are comfortable with the overall approach.
This PR is mainly refactors, with a few behavior changes and improvements:
- Introduces `TxPackageTracker`, which starts out as a wrapper for the `TxOrphanage`. This will be the m
...
💬 glozow commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-1664055615)
Opened #28199 with tests that may help us ensure this PR isn't changing specific behaviors. Marking this as draft as it depends on that one.
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-1664055615)
Opened #28199 with tests that may help us ensure this PR isn't changing specific behaviors. Marking this as draft as it depends on that one.