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

👍 instagibbs approved a pull request: "util: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1555428522)
ACK 76fe2c304201646d63b1b01b397e18a99252a2d9

played around with some extensions to see where it could go, I think longer term we could use `GenTxid` for situations where both are acceptable, and `Wtxid/Txid` when only one type is, and slowly migrate towards that. f.e. `GenTxid` could have a `GetTypedHash` to convert to the other.

💬 instagibbs commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279668749)
Let's use `Txid` here as well