💬 tdb3 commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642765022)
Yes, and overwrites the existing file if it does

💬 maflcko commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642765478)
See https://docs.python.org/3.8/library/functions.html#open

💬 brunoerg commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642768763)
Cool. Thanks.

💬 tdb3 commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642784926)
Yeah, at first glance showing `ALL,Failed` is not ideal, but in this case the CSV output is matching the approach taken by stdout printing, so it seems appropriate for now.

Since the scope of this PR is to focus on adding the CSV output capability, I'll leave it as-is for now. Perhaps a future discussion for how `ALL` is treated in both the stdout printing and the CSV could be had?

```
TEST | STATUS | DURATION

feature_blocksdir.py | ✓ Passed | 1 s
feature_conf
...

🤔 brunoerg reviewed a pull request: "test: write functional test results to csv"
(https://github.com/bitcoin/bitcoin/pull/30291#pullrequestreview-2122861389)
ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40

<details>
<summary>csv output</summary>
<br>
test,status,duration(seconds)
example_test.py,Passed,4
feature_abortnode.py,Passed,2
feature_addrman.py,Passed,5
feature_anchors.py,Passed,4
feature_asmap.py,Passed,8
feature_assumeutxo.py,Passed,45
feature_assumevalid.py,Passed,4
feature_bip68_sequence.py,Passed,7
feature_block.py,Passed,45
feature_blocksdir.py,Passed,1
feature_cltv.py,Passed,2
feature_coinstatsindex.py,Passed,7
feature_
...

💬 tdb3 commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642788308)
Good thought. Since the approach is to allow the user to specify the filename/extension as they see fit (rather than force a particular extension), this seems ok as-is. If others feel that forcing the extension `.csv` would be more ideal, then will keep this in mind.

💬 tdb3 commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2173355681)
> tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40)

Thanks for the review @rkrux!

🤔 marcofleon reviewed a pull request: "test: write functional test results to csv"
(https://github.com/bitcoin/bitcoin/pull/30291#pullrequestreview-2122866226)
Good idea, tested ACK ad06e68399da71c615db0dbf5304d0cd46bc1f40

💬 tdb3 commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2173359913)
> Good idea, tested ACK [ad06e68](https://github.com/bitcoin/bitcoin/commit/ad06e68399da71c615db0dbf5304d0cd46bc1f40)

Thanks for reviewing/testing @marcofleon!

💬 rkrux commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#discussion_r1642802719)
I see, makes sense to have that discussion separate from this.

💬 rkrux commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642824589)
Oh timeout as in expiration of the orphans. I assumed it referred to some kind of timeout of the insertion operation.
Makes sense to me now, thanks @glozow.

👍 vasild approved a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2122762881)
ACK 0e0c422aedd4009ab34eca127e4904d15e81f5be

Changes like this are inherently difficult to review:

One has to check all variables protected by `cs_main` and ensure that none of them are required to be in sync with the ones that are removed from `cs_main` protection like `m_txrequest`.

Also, one has to check that all mutexes locked before the new one are never locked after the new one anywhere else in the code and that mutexes locked after the new one are never locked before it anywhere
...

💬 vasild commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1642728827)
Since now `cs_main` need not be held when accessing `m_txrequest` better not hold it. For clarity and performance.

```diff
{
LOCK(cs_main); // For m_node_states
m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn()));
+ }
+ {
LOCK(m_tx_download_mutex);
assert(m_txrequest.Count(nodeid) == 0);
}

💬 vasild commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1642792214)
Should `s/invalid/valid`?

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2173431291)
Tested faacd264417bd7f3f08c5ff497458030b3a54fbc on Intel macOS 13.6.7, FreeBSD 13.2, Windows and Ubuntu 24.04.

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642862717)
I'm switching the word to "stored" to be clearer

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642862845)
good point, I added an assert to make it clear its larger, since I think that's the assertion we really want

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642862912)
considering this resolved

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1642751240)
a8818568a44e02c0346ef37cde9155191f8b3df1: `__FreeBSD_version` can be lowered to `1302000` as per the comment (or `1302001` which I tested on). Although you need `kldload /boot/kernel/netlink.ko`.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1642882599)
3fdc97a2bf7d1ec774d2ffa00502188158c64724: I don't know why this was here in the first place, seems fine to drop.