💬 MarcoFalke commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1659940762)
> It's not clear that is something that other project contributors agree with, i.e: discussion in this thread: https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1254980474.

I can see the desire to enforce it everywhere, but then it should be done via clang-tidy or a clang-tidy plugin. Ideally without modifying the source code at all.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280365390)
Mind linking to the doc in this line then? Otherwise all reviewers and future readers will have to wade through the docs themselves?

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1280373834)
> Well, it is still possible to get github.job property that has value macos-native from

`"macos-native"` as key seems fine (for ccache), no? (Any way is fine, though)

👍 Sjors approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556486530)
utACK 0280dc44d227ae34e8fcbb708833bfe9d66c7b5f modulo 6005165242cf090589934133f01f9dbd496612f1.

I also checked that all the intermediate commits compile and pass the tests (on Ubuntu 23.04).

I'm not familiar with the LevelBD code, but the refactor makes sense to me.

I'll look at 6005165242cf090589934133f01f9dbd496612f1 later since it might get simplified based on @MarcoFalke's suggestion.

💬 Sjors commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280311080)
74b5c3dac581b7597668a6a0cc55678d2af296da: `m_impl_batch` (since we don't use `p` for pointers) or `m_impl` (the pattern used in e.g. `AddrManImpl`)?

💬 Sjors commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280383910)
ca6f83d88cb241786676bfb341fc27f6467d39ee: this description, although not amazing, went missing

💬 fanquake commented on pull request "Move IsDeprecatedRPCEnabled to rpc/util, rm redundant rpcEnableDeprecated":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1659989600)
Needs a rebase, if still relevant, and the [question above](https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1539543097) also needs addressing.

👍 vasild approved a pull request: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078#pullrequestreview-1556652326)
ACK 4ecfd3eaf434d868455466e001adae4b68515ab8

I am also ok to add new functions in the same commit in which they are going to be used.

📝 MarcoFalke opened a pull request: " blockstorage: Drop legacy -txindex check "
(https://github.com/bitcoin/bitcoin/pull/28195)
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.

Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)

💬 MarcoFalke commented on pull request "test: Drop 22.x node from TxindexCompatibilityTest":
(https://github.com/bitcoin/bitcoin/pull/28070#issuecomment-1660034722)
Follow-up in https://github.com/bitcoin/bitcoin/pull/28195

💬 vasild commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1660036151)
_offtopic `[[nodiscard]]`_

There are 3 classes of functions wrt `[[nodiscard]]`:

1. where ignoring the result is harmful, it probably means a bug in the program, for example [write(2)](https://man7.org/linux/man-pages/man2/write.2.html)
2. where ignoring the result is ok and the code is equivalent without the call, aka it does not make sense to call the function, its call should be removed because it clutters the source code and makes the program slower, for example [strlen(3)](https://ma
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660036495)
> I'd be happy to work with a reopened #22242

I guess that pull is mostly orthogonal (shouldn't (silently) conflict with this one). Rebased and reopened in https://github.com/bitcoin/bitcoin/pull/28195

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660128648)
@MarcoFalke, I am not sure I fully understand in [your comment](https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387). Can you elaborate on the CPFP example?

In general, wrt the privacy of the transaction originator, do you think that the "sensitive relay" mechanism from this PR could be worse than the current "send to all" from `master` in some cases? Or do you think that it is better than `master` but it can be done even better?

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660192852)
> do you think that the "sensitive relay" mechanism from this PR could be worse

Yes, my understanding is that this pull strictly decreases privacy when the underlying wallet makes use of transactions chains (whether or not for intentional or accidental CPFP shouldn't matter), or transaction replacements, or mutually exclusive transactions ("conflicts", "double-spends" or insufficient rbf-fee), or if the transaction recipient is the attacker and can create transactions spending the "sensitive
...

👍 dergoegge approved a pull request: "refactor: Remove unused MessageStartChars parameters from BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/28191#pullrequestreview-1556884717)
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073

🤔 glozow reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1556437447)
Approach ACK, mempool sequence seems suitable for this.

I think we should add a functional test, perhaps [something like this](https://github.com/glozow/bitcoin/commit/258b1c8ced55b528d1afc585c23a3d98fa30394b)

💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280281473)
nit: this comment could be misleading after ffa081b007300f28fd9cffe6f4795c1125e2f373
```suggestion
const uint64_t entry_sequence; //!< Sequence number used to determine whether this transaction is too recent for relay.
```

💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280487599)
A comment might be helpful
```suggestion
/** Returns info for a transaction if its entry_sequence <= last_sequence. */
TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const;
```

💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280498101)
I think there may be an off-by-one here? The sequence number isn't incremented until `TransactionAddedToMempool()` fires later. Which means we could have:
t=0: mempool sequence is 1
t=1: tx A is submitted with `CTxMemPoolEntry` is constructed with `entry_sequence = m_pool.GetSequence() = 1`
t=2: main signals fires `TransactionAddedToMempool(A, m_pool.GetAndIncrementSequence())` with sequence 2
t=3: p2p announces A, `m_last_inv_sequence = m_mempool.GetSequence() = 2`
t=4: new tx B is submit
...

💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280500977)
nit: the commit message for ffa081b007300f28fd9cffe6f4795c1125e2f373 is "validation: when adding txs due to a block reorg, allow immediate relay" but the immediate relay behavior isn't until the subsequent commit when the bloom filter is removed