💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605211026)
> However, there is also a call to RemoveWallet, and UnloadWallet in this WalletCreate benchmark. The additional overhead of the remove_all should be fine? The benefit would be that the number of folders is kept constant and does not pile up as the bench runs, which could (in theory?) lead to issues when running for a long time?

hmm, I don't think they are comparable. The overhead of `remove_all` depend on external factors such us the OS and the hard drive. But agree that running this for a l
...

💬 maflcko commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2117871458)
utACK 5822a82a88e46ef08e5c18c41489ffd3e9d899c6

👍 maflcko approved a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2063792639)
utACK 7e30fdaa8fd7cc572505b7eac1b25f4ebe3b5443

💬 maflcko commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605220811)
```suggestion
auto wallet_path = fs::PathToString(test_setup->m_path_root / "test_wallet");
```

nit: For pure ascii, it is not needed

💬 sipa commented on pull request "Add option dbfilesize to control LevelDB target ("max") file size":
(https://github.com/bitcoin/bitcoin/pull/30059#issuecomment-2117888385)
I think it would be good to understand what the trade-offs are for this before considering making it configurable (as in: understand in what situations we'd want to recommend people use lower vs higher values). I assume lower values mean more open files/IO and less outdated data on disk, and higher values mean fewer compactions. Depending on whether those are the only effects, and to what extent these things are affected by the value, maybe we can get away with a default value that's a few % of
...

💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605221320)
No needed. I think I had the random path calculated outside of the loop before.
With the current code, neither the counter nor the random is needed. Thanks.

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