💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1720699892)
fixed on latest push - good to have people not relying on github for code review.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1720702488)
The `ExpectedTx` have been updated to look by peer {peer, false, txhash} rather than iterating as we’re asking information for a given peer. I’ve not changed `ReceiveResponse` function call type, to avoid having unused value compiler errors, neither benchmarked yet to compare more the 2 approach.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2294752502)
I think the main open problem of this approach, and iiuc as pointed out by gmaxwell and aj, if it’s applied to upgraded peers only (`REJECT_UNSOLICITED_TX_VERSION`) is the occupation of inbound slots by buggy peers. Slots that could be rather used by more non-buggy peers to preserve robustness of the transaction-relay topology.

I think one way to alleviate is to introduce a new versioning of the transaction-relay protocol itself, which has not been really existent so far and that way have an
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256)
Rebased due to the conflicts. This PR branch is the current [staging branch](https://github.com/hebasto/bitcoin/commit/818a3c07fb48e4f7fdaf5640f3987d47280f54d0) with the top 3 commits dropped.

Some feedback has been addressed including:
- @fanquake's https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277722262
- @fanquake's https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777
- @pablomartin4btc's https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2234
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737320)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737774)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737851)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737884)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737895)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294789962)
@whitslack

> ACK [67b1e23](https://github.com/bitcoin/bitcoin/commit/67b1e236334f38ec4e4d2251dbdfb790f20ed88b) on Gentoo, modulo our usual [hacks](https://github.com/whitslack/bitcoin/commit/cmake-syslibs) (now adapted to CMake) to allow linking against system-installed libsecp256k1 and LevelDB.

Thanks for testing!

> One small gripe: When we enter the install phase, CMake always rebuilds `src/clientversion.cpp`, which forces a relink of all target executables. It would be better if `ni
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294793259)
During the recent CMake Working Group call, it was agreed to freeze this branch until it is merged, unless a severe bug is reported or a merge conflict arises. All other reported issues will be addressed in follow-up PRs.

Please continue testing this branch as thoroughly as possible.

💬 paplorinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2294805824)
ACK eff5bf7d673c797d63c5ad15ac18b8316dea5ffe
(diff only contains `Evaluate*` -> `EvaluateFee*` changes)

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2294818443)
Thanks, added that (correct) sentence to the description.

💬 pythcoiner commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2294821305)
reACK a0abcbd

🤔 paplorinc reviewed a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2244077893)
I'm mostly fine with it, would love if we could make a few usages more compact, since I think we've lost readability a bit

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720755562)
Given that
```C++
assert(Vec(HexLiteral<uint8_t>("01")) == valtype{1});
assert(Vec(HexLiteral<uint8_t>("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")) == valtype(36, 0xff));
```
consider simplifying these to e.g.
```suggestion
t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << valtype{1} << 2 << valtype(36, 0xff);
```

and many more, I think it would simplify a few trivial usages, hexadecimal encoding only makes sense for bigger values.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720757880)
Is `Vec` needed here because the existing `ToByteVector` has a long name? Or because of some funny visual studio exception? If none, maybe we could get rid of either.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720760053)
nit: as stated before, the trailing `\` can likely be removed.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720769066)
Given that we already seem to be using custom string overloads (e.g. https://github.com/bitcoin/bitcoin/blob/master/src/script/miniscript.cpp#L87), would it maybe make the usages less noisy if we tried something like:
```C++
consteval auto operator""_hex(const char* hex_str, const std::size_t len)
```
to be able to write `"0000deadbeef0000"_hex` instead of `HexLiteral<uint8_t>("0000deadbeef0000")`?

I haven't spent a lot of time with this to make it work since I'm not yet sure it's a good
...

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720750629)
super-nit: during parsing we're checking now that the inputs contain an even number of hexadecimal digits, if you edit again, consider applying that to the comments as well (similarly to other comments, like `0x02ff03`)

```suggestion
s = ToScript(HexLiteral<uint8_t>("0302ff030302ff03")); // PUSH 0x02ff03 PUSH 0x02ff03
```