💬 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.
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283271742)
It's not that important :shrug: it can be a real child. I wrote the test trying to illustrate a spy peer trying to get information, i.e. one that doesn't have the ability to spend the transaction's UTXOs.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283271742)
It's not that important :shrug: it can be a real child. I wrote the test trying to illustrate a spy peer trying to get information, i.e. one that doesn't have the ability to spend the transaction's UTXOs.
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283274009)
(I didn't change it but I also don't feel strongly, could combine if you want)
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283274009)
(I didn't change it but I also don't feel strongly, could combine if you want)
💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283274749)
nope it's fine
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283274749)
nope it's fine
💬 instagibbs commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#issuecomment-1664086042)
ACK https://github.com/bitcoin/bitcoin/pull/27832/commits/faafc35a779745d59fdb0e88698b579215f42b08
(https://github.com/bitcoin/bitcoin/pull/27832#issuecomment-1664086042)
ACK https://github.com/bitcoin/bitcoin/pull/27832/commits/faafc35a779745d59fdb0e88698b579215f42b08
💬 instagibbs commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664092785)
> Can it be used to pin transactions in a way that unupgraded nodes (including poorly maintained alternative implementations that are potentially used in second layer protocols) wouldn't notice?
I don't see how this would result in pinning. Pinning is of two general flavors:
1) economic/rule#3 pinning: costly to RBF even though miner incentive would be to pick it up
2) package limit pinning: ancestor/descendant size/count limits are hit by adversary, so you cannot CPFP
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664092785)
> Can it be used to pin transactions in a way that unupgraded nodes (including poorly maintained alternative implementations that are potentially used in second layer protocols) wouldn't notice?
I don't see how this would result in pinning. Pinning is of two general flavors:
1) economic/rule#3 pinning: costly to RBF even though miner incentive would be to pick it up
2) package limit pinning: ancestor/descendant size/count limits are hit by adversary, so you cannot CPFP
💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1664121698)
Updated f26396732b940095380d76ed77685e0fb11294b8 -> 3fb2dac2ce78092c1006ee082c536bea1b69a152 ([cleaveLeveldbHeaders_3](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_3) -> [cleaveLeveldbHeaders_4](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_3..cleaveLeveldbHeaders_4))
* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283223804)
...
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1664121698)
Updated f26396732b940095380d76ed77685e0fb11294b8 -> 3fb2dac2ce78092c1006ee082c536bea1b69a152 ([cleaveLeveldbHeaders_3](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_3) -> [cleaveLeveldbHeaders_4](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_3..cleaveLeveldbHeaders_4))
* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283223804)
...
💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1283311586)
> So the spy has to try to connect back to the node copying the nonce before they send ther VERACK, ie before they know this is a sensitive relay connection.
They know it's likely a sensitive relay connection based on the version message they receive. So they withhold their verack on that connection and then keep connecting and sending version messages to all public addresses until they found the node that disconnects them for sending the right nonce. After that they can send the verack and r
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1283311586)
> So the spy has to try to connect back to the node copying the nonce before they send ther VERACK, ie before they know this is a sensitive relay connection.
They know it's likely a sensitive relay connection based on the version message they receive. So they withhold their verack on that connection and then keep connecting and sending version messages to all public addresses until they found the node that disconnects them for sending the right nonce. After that they can send the verack and r
...