💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1605180790)
done in #29431 actually

💬 maflcko commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2117837985)
> Do you have specific output or details?

The DrahtBot builds (as well as the log outputs) are deleted by now, as they are short-lived test-only assets. I'll create fresh DrahtBot builds and try again with the current state of this pull request.

💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2117847337)
> I've overridden some default behaviour of the configure script like: --prefix=$HOME --with-incompatible-bdb --with-gui=no
>
> "Next, run the configure script to automatically discover all the necessary libraries and create a customized build script for your system:"
>
> $ ./configure

test_bitcoin-qt is a GUI executable. Are you sure you passed `--with-gui=no`?

Please share the configure summary after you disable the gui:

```

./configure --prefix=$HOME --with-incompatible-bdb
...

💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2117848777)
It should say something like:

```
Options used to compile and link:
external signer = yes
multiprocess = no
with wallet = yes
with sqlite = yes
with bdb = no
with gui / qt = no
...

💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2117851678)
Also, is there a need for you to be using BDB? It is deprecated and going forward it will be easier for you if you used sqlite from the start, if you do not need backward compatibility.

💬 maflcko commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1605200936)
nit: For new code it would be good to use `LogInfo`, not the deprecated and confusing alias `LogPrintf`.

💬 tdb3 commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1605207235)
In general, for tests I think the inclination should be to get data from direct sources (e.g. RPC) rather than from the debug log, but this instance seems like an edge case where searching the debug log might make sense (e.g. could match what's in the log with what the RPC call response contains).

I briefly looked for this capability, but didn't see it (I was surprised, so maybe I'm overlooking something simple). If others would like this capability, I'm thinking it's probably something to h
...

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