💬 ryanofsky commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2294347140)
I lost enthusiasm for this PR, so will close it, but tagging as up for grabs, and will happily review if someone else wants to pick it up and respond to the earlier comments, particularly the individual logging improvements suggested https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2182052463.

---

The thing I like about this PR is that it disentangles log message ___priority levels___ from log message ___conditions___.

As I see it, **priority levels** can distinguish bet
...

💬 ryanofsky commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2294365452)
re: https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2231748835

I think I might not have enough information to understand the weak nack, because I don't actually see a reason in there to reject a documentation fix that is only dropping advice to interpret `Error` as "fatal" and `Warning` as "nonfatal".

The advice is not followed by current code, but more importantly, I don't think anybody (even the original author) currently thinks this advice is something we should try to implem
...

👍 ryanofsky approved a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-2243634116)
Code review ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f

💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1720427422)
re: https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1719563783

> q in [f1c1b37](https://github.com/bitcoin/bitcoin/commit/f1c1b37d2247eed49156c0467daa68aa38497bb8): Can you explain why this change is correct?

Of course it is not correct. I only noticed that this function was updating `g_insecure_rand_ctx` and didn't notice it was updating global rng state. Thanks for fixing!

👍 ryanofsky approved a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2243660929)
Code review ACK 164732369fc5ecf4c7705136a2de884419023b5a. New commit replacing SeedRandomForTest is very nice and clarifies the code.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720368838)
Was removed it in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716588998 but then I went one step further and deferred the assert to the `base_blob` `Span`-ctor, making the comment more relevant again. Brought back now.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720392370)
Started going down the route of `std::span<const unsigned char, 32>` to enforce the constraint previously, got burnt and backed out a bit too far, `std::span` with dynamic extent and comment will do for this PR. :+1:

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720396199)
Leftover from half-way rename, will fix here and function below.

💬 whitslack commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294461104)
ACK 67b1e236334f38ec4e4d2251dbdfb790f20ed88b on Gentoo, modulo our usual [hacks](https://github.com/whitslack/bitcoin/commit/d028d85b4e9ae99067968fe480bd0eefe06c8be0) (now adapted to CMake) to allow linking against system-installed libsecp256k1 and LevelDB.

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 `ninja install` would not rebuild anything.

💬 Sjors commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2294705913)
re-ACK 221809b81cfcecb04050915eebacffda2599da42

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2294734148)
Addressed all code comments at c7bc444d02

——————————————

@gmaxwell

> That's like not-found, -- an idea that sounds attractive on it's face but on examination causes problems. For behavior that can't be prevented (like, say, inv-ing a transaction to you that already know), then it should obviously never disconnect for that. For behavior that can be prevented like sending a txn before an inv, sending an invalid block, etc.., peers are either compatible or they're not. If they're not you w
...

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