💬 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

💬 instagibbs commented on pull request "test: tx orphan handling":
(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

📝 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
...

💬 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.

💬 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.

💬 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)

💬 instagibbs commented on pull request "test: tx orphan handling":
(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

💬 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

💬 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)
...

💬 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
...

🤔 stickies-v reviewed a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1560934462)
light crACK 3fb2dac2ce78092c1006ee082c536bea1b69a152

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283098892)
nit: that's a pretty long line

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283281451)
I think this needs to be removed?

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283280722)
nit: pretty long line

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283260447)
It's not immediately obvious to me why we can't stick to our usual pattern of using a `const DataStream& ssKey, CDataStream& ssValue` function signature, and resort to lambdas as is done here. I think the below code is much more straightforward, but perhaps I'm missing some nuance or drawbacks?

<details>
<summary>git diff on f26396732b940095380d76ed77685e0fb11294b8</summary>

```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 1b2548f262..cb27b739fe 100644
--- a/src/dbwrapp
...

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283286314)
nit: I think this needs a `LIFETIMEBOUND`.

Also: suggested alternative name `DBContextRef()` to highlight that the return value can mutate our `CDBWrapper` state? No strong view on it though, happy with the current one too, just a suggestion.

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283298874)
nit: Would this be an improvement, to highlight the reference nature of `db_context`, as well as I think generally it's slightly cleaner to not call `DBContext()` every time (I know it's a very cheap call)?

```suggestion
auto& db_context{DBContext()};
db_context.penv = nullptr;
```

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1283315877)
nit: would it make sense to move this to the `LevelDBContext` destructor?