💬 Mazzika1 commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1642252570)
Okay

💬 Mazzika1 commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1642252790)
Okay

👍 rkrux approved a pull request: "test: Added test coverage to listsinceblock rpc"
(https://github.com/bitcoin/bitcoin/pull/30195#pullrequestreview-2121134118)
tACK [881724](https://github.com/bitcoin/bitcoin/pull/30195/commits/881724d443d11f984a721ef1edd5777c24d1ed29)

Make successful, so are all the functional tests. Asked a question for my clarity.

💬 rkrux commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1641643772)
Super Nit: `node1_last_blockhash`

💬 rkrux commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1642285756)
I've been trying to understand how does calling `listsinceblock nodes1_last_blockhash` on `node0` is causing the last condition to be true here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L663

The comment above this line talks about requesting a reorg'ed block, but isn't the last blockhash on node0 part of the main chain from POV of node0?

👍 rkrux approved a pull request: "test: write functional test results to csv"
(https://github.com/bitcoin/bitcoin/pull/30291#pullrequestreview-2122088460)
tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40)

Make is successful so are all the functional tests. Passed the `resultsfile` param and generated the results csv few times.

Thanks @tdb3 for this, it's a good addition to the testing suite.
I can imagine using this feature and also sorting the tests in descending order based on time taken. I mentioned couple points but none are blockers.

💬 rkrux commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642322154)
The write function below `write_results` intentionally creates a csv file, should we add a check here for the `csv` file extension?

💬 rkrux commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642326303)
As soon as the `functional_test_results.csv` file was generated in my system, I thought of adding it in `gitignore`. Since this feature is conditional based on the param, we can't add this hardcoded name in gitgnore but how about adding `*.csv` in gitgnore?

💬 rkrux commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642313591)
Nit: Having had only few tests failed also display `ALL,Failed`, which in the first glance gives the impression that all of them failed. In cases like this, displaying `ALL, Few failed` would be more explicit.

💬 S3RK commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-2172540718)
I think there is a silent merge conflict with #29325

Otherwise, to the best of my understanding, the code is implemented according to [BIP-326](https://github.com/bitcoin/bips/blob/master/bip-0326.mediawiki) and LGTM

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2172702822)
> any other good ways you'd like to see these changes exercised `getblocktemplate()` is a pretty crucial RPC

You could test #27433 on top of the changes here. Or try solo CPU mining on a custom signet, e.g. using [pooler/cpuminer](https://github.com/pooler/cpuminer) and then:

```sh
./minerd -u bitcoin -p bitcoin -o http://127.0.0.1:38332 --coinbase-addr $ADDRESS --no-stratum --algo=sha256d
```

The latter requires `signetchallenge=51` and a patch setting `#define GBT_RULES "[\"segwit\"
...

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1642458226)
`GetBlockHash()`, which was called directly before this commit, has an `assert` that `tip` must exist. So `return uint256{0};` should never happen. An optional does seem more robust though, will look into it.

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1642459287)
I added a default `true` to mimic `TestBlockValidity` in `validation.h`.

👍 rkrux approved a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2122365305)
tACK [0aa7db4](https://github.com/bitcoin/bitcoin/pull/29862/commits/0aa7db42564408edb41b0d42103d39ba4c2787dc)

`make` and `make check` successful, so are all the functional tests.

Thanks for adding these unit tests and using `SCRIPT_VERIFY_NONE` instead of hardcoded 0.

💬 cdecker commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2172871011)
I spoke with @emzy and I decided to revive this seed, and migrate to a more standard deployment as well. The overhead of the custom solution did not add enough quality, and caused the seeder to rot and break. Using sipa's seeder will solve that, and reduce the burden of maintenance.

No timeline yet, but I'll dedicate a couple of weekends

👍 hebasto approved a pull request: "ci: parse TEST_RUNNER_EXTRA into an array"
(https://github.com/bitcoin/bitcoin/pull/30244#pullrequestreview-2122406132)
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f, tested on Ubuntu 23.10 and Windows 11.

In the command prompt on Windows, the following commands work as expected:
```
>set EXTRA=--exclude "rpc_bind.py --ipv6, feature_proxy.py"
>py -3 test\functional\test_runner.py %EXTRA%
```

which is sufficient for our CI if needed.

⚠️ Fonta1n3 opened an issue: "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs."
(https://github.com/bitcoin/bitcoin/issues/30294)
### Please describe the feature you'd like to see added.

I'd like an option to remove the `bip32derivs` from outputs as well as inputs when calling `walletprocesspsbt`.

### Is your feature related to a problem, if so please describe it.

I am using Bitcoin Core to create psbt's which are meant to be passed around to multiple users for collaborative transactions. I want to leak as little data as possible. When I set `bip32derivs` to `false` with `walletprocesspsbt` it does not remove the `bip32
...

💬 marcofleon commented on pull request "fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read":
(https://github.com/bitcoin/bitcoin/pull/30273#discussion_r1642560055)
You can add it to the end of the command. So `FUZZ=i2p ./src/test/fuzz/fuzz -dict=DICTFILE`. The dictionary (`i2p.dict`) is in the qa-assets repo: https://github.com/bitcoin-core/qa-assets/tree/main/fuzz_dicts. I'm running it now on the most recent commit.

👍 dergoegge approved a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237#pullrequestreview-2122540242)
tACK 09b90c69bc17ce43dce3881a208d9066a729c67d

💬 maflcko commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2173079380)
re-ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40