🤔 maflcko reviewed a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2412663196)
left some nits on the first commit. Will take a look at the others later.

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827490394)
Any reason to log the file name here? The containing folder of the log should already have the name of the test/file. If not, the user should know which test they called to know it.

If not, and this was helpful, it would be better to log it for all tests, instead of just for this one test specifically.

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827592298)
I don't think this is true, it is just one example. I think 342 can be raised by authproxy.py for various reasons.

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827578786)
nit in 5ca92b17c5d2d5efa7b2246da1905bb8fd186230: It is obvious, but could mention the success case as well?

`Retry after these, until a permanent success or failure state (such as FailedToStartError, or timeout) is reached.`?

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827600606)
nit: Could just mention that the three are treated equal to "-342 Service unavailable", which is already explained above?

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1827605564)
nit: I don't think "bitcoind is still starting" is true either. It is just one example. Another example would be that the cookie file is (accidentally) disabled in the config.

What about just `Retry if cookie file is not created and no rpcuser or rpcpassword."?

💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2460475259)
I've taken this a little bit further, by making the debug logging uniform too.

✅ secp512k2 closed a pull request: "doc: Add missing 'blank=true' option in offline-signing-tutorial.md"
(https://github.com/bitcoin/bitcoin/pull/31236)

📝 secp512k2 opened a pull request: "doc: Add missing 'blank=true' option in offline-signing-tutorial.md"
(https://github.com/bitcoin/bitcoin/pull/31237)
Issue:

The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option.

Correction:

Added `blank=true` to the command to match the options described in the text.

Explanation:

The `blank=true` option is necessary to create a blank wallet. Including this option ensures the command matches the options specified in the text.

<!--
***
...

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1831538454)
You are right. If ``data.nTime`` is bigger than ``end_of_chain_timestamp`` we could go into the case where ``fTxTotal`` is smaller than ``pindex->m_chain_tx_count`` .

Maybe @sipa can explain why thinks that after the changes this clamp could be removed? As mentioned in https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432418782.

For the moment, I will add it again. Thanks for the observation!

🤔 ismaelsadeeq reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2419167788)
Concept ACK

📝 maflcko opened a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238)
I don't think the fuzz target has ever found a real issue. The closest being https://github.com/bitcoin/bitcoin/pull/25869

It is also, by far, the slowest fuzz target. For example, looking at https://cirrus-ci.com/task/5533338067271680?logs=ci#L3974, it takes more than one hour:

```
Run wallet_notifications with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/wallet_notifications
...

💬 achow101 commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2460749631)
ACK d22a234ed270286b483aec2db1e2f716b9756231

💬 l0rinc commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2460759546)
@maciejsszmigiero, are you still working on this or should we take over?

---

I can also confirm that it's possible to just switch file size values back-and-forth without needing a reindex.
I have reindexed until block 600k with master vs 16 mb blocks (instead of the 128 for the reasons mentioned before).

The LevelDB files seem to effortlessly change from 2 mb to 17 :
* **chainstate/062435.ldb - 906'412 bytes**
* chainstate/061885.ldb - 2'171'330 bytes
* chainstate/063212.ldb - 1'936
...

🤔 mzumsande reviewed a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2419408412)
Concept ACK

I agree with the general motivation. While this PR can change the error message of `submitblock` in case more than one thing is wrong with the block, I don't really see why one is more important than the other.

> UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex which would result in UB, before any PoW checks are made.

Nice find!
I think that it would be good to have a test that would trigger the UB if https://github.com/bitcoin/bitcoin/commit/ada0caa165905b50db
...

🚀 achow101 merged a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164)

✅ achow101 closed a pull request: "tests: Handle BDB dynamic pagesize"
(https://github.com/bitcoin/bitcoin/pull/31222)

💬 achow101 commented on pull request "tests: Handle BDB dynamic pagesize":
(https://github.com/bitcoin/bitcoin/pull/31222#issuecomment-2460788291)
> Note that a similar change is also included in #30125,

Oh, right, forgot about that PR. Closing in favor of it.

💬 achow101 commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#issuecomment-2460794766)
ACK d45eb3964f693eddcf96f1e4083cf19d327be989

💬 brunoerg commented on pull request "fuzz: Limit wallet_notifications iterations":
(https://github.com/bitcoin/bitcoin/pull/31238#issuecomment-2460821784)
Besides being the slowest, I think this target has poor stability, around 40%.
cc: @marcofleon

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460858505)
> I've interpreted (and implemented) it as also allowing the taproot internal key, not just the output key. I think that is actually what I meant when writing the BIP, but it's been a while.

I think it's important that the (aggregate) plain key used in the key of `PSBT_IN_MUSIG2_PUB_NONCE`/`PSBT_IN_MUSIG2_PARTIAL_SIGNATURE` is unambiguous and clearly specified, or implementations will essentially have to try both for both in order to find in the PSBT all the pubnonces/musig_partial_signatures
...