💬 tdb3 commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2172107885)
> Concept ACK
>
> This line initially when I read it made me assume that 28 tests failed when in reality 28 passed and one failed `ALL,Failed,28`
>
> Also I think the durration(seconds) here is equal to number of tests for all which seems to be inaccurate too
>
> I would expect
>
> ```
> test,status,duration(seconds)
> feature_blocksdir.py,Passed,1
> feature_config_args.py,Passed,28
> wallet_startup.py,Passed,2
> mempool_accept.py,Failed,1
> ALL,Failed,32
> ```

Thank you fo
...

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1642205744)
agree! done.

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1642208728)
oh looks like 0 garbage bytes being returned can also mess up the functions where we're tampering the garbage bytes (`WRONG_GARBAGE`) and `SEND_NO_AAD` where AAD is the garbage bytes sent. so I've change `generate_keypair_and_garbage()` in `EncryptedP2PState` to return 1 bytes garbage minimum instead.

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1642204643)
`PEP 8: E303 too many blank lines (2)` was showing up in my code editor. so maybe we can keep it since we're touching the function here.

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

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

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