💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096505344)
> > Prevent use of the 20-minute difficulty exception on the last block in a difficulty period
>
> @murchandamus do you explicitly prefer this over my proposed fix? I find it more elegant and intuitive if we allow the use on all blocks and prevent the adjustment from being affected by it.

> > Alternatively, I would be satisfied if using the 20-minute difficulty exception would be changed such that nBits could remain at the proper value, and were available for any block in the difficulty pe
...

💬 sr-gi commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2096517368)
I find the approach fairly hard to follow here. Having all the logic in the constructor with if/elses based on the connection type instead of having different constructors/a test for each type of fail feels really error-prone and difficult to make sure if a test is doing what we expect. I think it'd be better to have multiple classes that build from `v2_p2p` and modify what's needed, even if the file gets larger.

Just an example:

In the `WRONG_GARBAGE_TERMINATOR` case, we are modifying the
...

👍 ryanofsky approved a pull request: "Logging cleanup"
(https://github.com/bitcoin/bitcoin/pull/29798#pullrequestreview-2041268723)
Code review ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc. Looks good, this gets rid of some unnecessary complexity and strange behavior in logging RPC.

💬 ryanofsky commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1591311644)
> > Right now this returns NONE/true when the empty string "" is passed.
>
> Hmm, no? It returns ALL/true when "" is passed, in `master` and in this PR - there is an `if (str.empty() ...` a few lines above.

Right, I think I was confused when I wrote that comment. Makes sense to keep "" behavior unchanged, too.

👍 theuni approved a pull request: "net: Replace ifname check with IFF_LOOPBACK in Discover"
(https://github.com/bitcoin/bitcoin/pull/29984#pullrequestreview-2041299260)
utACK a68fed111be393ddbbcd7451f78bc63601253ee0. Satoshi-era brittleness :)

💬 theuni commented on pull request "depends: sqlite 3.45.3":
(https://github.com/bitcoin/bitcoin/pull/29991#issuecomment-2096548294)
Any particular reason for the bump?

⚠️ achow101 opened an issue: "Memory leak with `rest/block` REST endpoint and `getblock` RPC when verbosity >=2"
(https://github.com/bitcoin/bitcoin/issues/30052)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Reported in https://bitcointalk.org/index.php?topic=5495435.msg64037741#msg64037741

When using either the `rest/block` endpoint or the `getblock` rpc with verbosity >=2, there appears to be a memory leak. Memory usage goes up ~100 MB for each call.

The original reporter is repeatedly hitting the rest endpoint and eventually OOM'ing.

### Expected behaviour

Memory should be freed aft
...

🤔 theuni reviewed a pull request: "crypto, refactor: add method for applying the taptweak"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2041367317)
This function feels kinda weird. As it's named `ApplyTapTweak`, I would expect it to modify `*this`. It also takes in a `uint256* merkle_root` but doesn't check for null, a reference would make more sense.

Perhaps this would make more sense as a static utility function that takes input/output keys?

💬 theuni commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2096614824)
I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?

💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2096670310)
Regarding the custom lib/self-impl discussion in #30005:
Taking a quick look at the implementation here, I think it's simple enough for us to maintain ourselves. If a nice canonical lib ever emerges we could always jump to it, as there are only a few basic functions and presumably we could probably switch them out close to 1:1.

That said, it is a little rough to review as
- It needs to be compared to the letter of the spec, with the assumption that @laanwj is evil or has mis-implemented (I
...

💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1591418368)
Aren't these checks broken if limit is `NO_LIMIT` (0) ?

💬 katesalazar commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2096770530)
Concept ACK

💬 TheBlueMatt commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2096783647)
I put this elsewhere but figured I should copy it here:

One thing that was done at the start of testnet3 was to include a reasonable amount of test coverage of unique scripts. This was, at a minimum, the Bitcoin Core script tests that were available but probably included scripts from other projects as well. As a result, just syncing the first few thousand blocks of testnet3 is a reasonable test case for anything validating scripts.

I'd strongly encourage the same be done for testnet4, but
...

🤔 furszy reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2041559101)
ACK 75d27fefc with a non-blocking nit.

💬 furszy commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1591483535)
nit: It wouldn't hurt to add a comment above this disconnection (just mention how this behaved historically).
I think that people reading this code for the first time will not understand why the peer is being disconnected for a node local failure.

💬 andrewtoth commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1591530770)
There is a comment a few lines up that describes the behavior https://github.com/bitcoin/bitcoin/pull/26326/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2453. But I agree, and will add that same comment here if I need to touch this up again.

👍 theuni approved a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2041686542)
Very nice change. ACK a885a166cec6d84d08600f12b25d912bd28af80e

💬 theuni commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1591560587)
This is really unfortunate, but I can't think of anything less hack-ish. And at least it's relegated to the test code.

🤔 furszy reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2041703888)
Going slowly here, I want to go through the bdb code diffs too. But, so far, so good. Benchmark 8f2267ff24acdc shows no diff with master and also have created and migrated a v0.6.3 wallet without any problem.

💬 theuni commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1591572104)
Just noticed while reviewing #29252...

This was removed, but the `#include <util/signalinterrupt.h>` include wasn't. I can open a quick cleanup PR, or would you like to @ryanofsky ?

(Looks like `<memory>` can go too.)