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

💬 instagibbs commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664147208)
> This proposed change simply cleans up a bit of remaining standardness paternalism; we've gotten rid of essentially all other standardness paternalism. I can't think of any standardness mechanism other than OP_Return and first-seen (which I'm arguing to remove by default in https://github.com/bitcoin/bitcoin/pull/28132) that doesn't have a strong technical reason to exist.

I'm not going to war for this change but this strikes me as correct. I classified the OP_RETURN limit as the last remain
...

💬 RobinLinus commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1664150253)
> Why do you believe that expanding a publication mechanism that is 4x more expensive than the heavily used taproot witness mechanism will "increase the spam problems significantly"?

@ChristopherA just said that he's paying you to push this because it makes it cheaper and easier for him to write his non-bitcoin stuff into the chain.

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1283345857)
OK I see. In that case, it can be a follow-up. Probably there is some reason we keep the scheduler and don't resend right on startup (we wouldn't want spy nodes detecting our restart! ...?)

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283379722)
FWIW, I made this non-default because I don't think it's a good idea for a vanilla `make` to spew warnings _on purpose_. It's bound to set off c-i somewhere eventually.

As @MarcoFalke points out though, we do want it run when we're in control of the environment and not worried about those warnings. In order to run them, I suggest:
```diff
diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index 75d5469267..02358db789 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b
...

💬 pinheadmz commented on pull request "test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28154#discussion_r1283381371)
I kinda feel like you don't need to use the helper for this one. It's actually +2 lines as opposed to -2 in the other locations.

🤔 pinheadmz reviewed a pull request: "test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28154#pullrequestreview-1561373322)
code review ACK at 83d7cfd5429b0be7755bc48145032956f5f56dae

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664210830)
@fanquake Would you mind removing the single use of `NOLINT` and pushing, just so that we can make sure the c-i still fails as expected after all the changes?

Assuming it does, LGTM either with or without addressing [this comment](https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279354069).

💬 theuni commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1664234300)
Concept ACK. I didn't review the includes changes in detail, but the move makes sense to me. And I'm always up for one-data-structure-per-header :)

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1283423591)
Added in the current push.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1664258585)
> Would you mind removing the single use of NOLINT and pushing, just so that we can make sure the c-i still fails as expected after all the changes?

Done.