💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605221532)
done

💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605223959)
done. thanks

💬 maflcko commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2117896947)
utACK 7c8abf3c2001152423da06d25f9f4906611685ea

👍 theStack approved a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2063616475)
Code-review ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019

💬 theStack commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1605118054)
It took me a while to grok what's going on here: if the CScript was already contained in the wallet, the `AddCScript` call one line above would fail in the course of writing to the DB (looking at the call flow `LegacyScriptPubKeyMan::AddCScript` -> `LegacyScriptPubKeyMan::AddCScriptWithDB` -> `batch.WriteCScript(...)` -> `WriteIC(..., ..., false)`, where the last `false` parameter indicates that overwrites are not allowed; the `FillableSigningProvider::AddCScript` call earlier doesn't care if an
...

💬 theStack commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1605226835)
nit (feel free to ignore): imho keeping the `nsigs`/`nkeys` naming here makes sense, to make it clearer that these are counts, not actual signatures and keys

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605249634)
> Any reason to have a duplicate None check for peer at this point? Shouldn't it be enough to have the assert in the second-to-previous line?

Not sure if I fully grasped the question but if the peer gets disconnected for some reason in-between these two checks, this wait_until call would throw an exception as the code would be trying to use the brackets operator on a `None` value to access the "bytesrecv_per_msg" value.

📝 edilmedeiros opened a pull request: "Increase contrib/signet/miner search space"
(https://github.com/bitcoin/bitcoin/pull/30130)
The miner script will call `bitcoin-util grind` to compute PoW which will try to exhaust the block's nonce field and fail if it can't find a valid hash. This behavior does not appear for low difficulty chains, but make the miner unusable for higher difficulty settings.

We capture `bitcoin-util grind` failure, build a new block header by changing the block time and try to grind again.

Fixes #30102.

## How this was tested

This is a follow-up from #30091 and #30102.

I've started a ne
...

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2117932334)
Force pushed so that the commit message is the same as the PR title.

👍 stickies-v approved a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2063857291)
ACK b47bd959207e82555f07e028cc2246943d32d4c3

💬 stickies-v commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1605255453)
nit: perhaps `static SynchronizationState GetSynchronizationState(const ChainstateManager& chainman)` makes for a more logical function signature now, but it doesn't really seem to make for much of a clean-up either, so... not sure.

💬 theStack commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2117946696)
Concept ACK

Can you squash the two commits? (Currently compiling d06227fd6b87b5a427441a212c04a0ba64edc142 fails).

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1605278661)
Mmh, I think I prefer this as is.

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2117967175)
A small caveat: I couldn't get rid of the `Could not satisfy difficulty target` (see the debug log just before timestamp `2024-05-17 12:56:01`). This is emitted by `bitcoin-util` itself before failing. I could not find a way to make the Python script intercept that to keep the terminal clean. I fear the complexity to achieve such an effect would not justify the benefit.

👍 theuni approved a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095#pullrequestreview-2063982122)
Agree with @laanwj's take.

utACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d

👍 theuni approved a pull request: "build: Enable `thread_local` for MinGW-w64 builds"
(https://github.com/bitcoin/bitcoin/pull/30099#pullrequestreview-2063984747)
utACK 5822a82a88e46ef08e5c18c41489ffd3e9d899c6.

After this and #30095, is there any need to keep the thread_local option at all?

💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2118018627)
@willcl-ark Thanks, that's very helpful. Out of curiosity, what build options are you using for these? Are optimizations on?

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605307850)
```suggestion
"Usage: bitcoind [options] Start " PACKAGE_NAME "\n"
```

🤔 edilmedeiros requested changes to a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2063952610)
Concept ACK

I would keep the old comment for each command even in face of the additional information provided (see review comments). I tend to rely a lot on those to quickly remember what the commands do (but will not oppose taking them out if this is the consensus). I understand that they may break the formatting of the manual pages, so feel free to discard them.

For the `bitcoin-util` tool, I suggest taking the description from the PR that introduced the tool (see comment).

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605316111)
```suggestion
"Usage: bitcoin-tx [options] <hex-tx> [commands] Update hex-encoded bitcoin transaction\n"
```