👍 vasild approved a pull request: "test: avoid internet traffic in rpc_net.py"
(https://github.com/bitcoin/bitcoin/pull/31343#pullrequestreview-2457645577)
ACK 988721d37a3c65e4ef86945133207fda243f2fba

The dummy proxy would also make it impossible to establish connections to the nodes running on localhost. E.g. `nodes[0]` now will not be able to connect to `nodes[1]`. I did not check the entire `rpc_net.py`, but I assume this must be ok, because the test passes with this setting.

💬 laanwj commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2497252843)
Concept ACK

i'm slightly worried this may generate false positive. As is, this detects traffic on the entire (virtual) machine while running the tests. Are there no other daemons running on the CI instance that could interfere with this?

💬 laanwj commented on pull request "cmake, qt: Use absolute paths for includes in MOC-generated files":
(https://github.com/bitcoin/bitcoin/pull/31361#issuecomment-2497265418)
Concept ACK, will test!

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856145428)
I drafted a comment, but I think it's wrong so I haven't pushed it yet:

```cpp
/* Wait for genesis block to be processed. Typically kernel_notifications.m_tip_block
* has already been set, except in three cases where the genesis block is
* being connected by the initload thread:
*
* 1. first startup with an empty datadir
* 2. reindex
* 3. reindex-chainstate
*/
```

I don't think the `initload` thread ever sets `kernel_notifications.m_tip_block
...

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2497287169)
I added a check to ensure we're not the background chainstate when sending the notification.

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856151977)
It's clearly doing something though, because the following breaks several functional tests:

```patch
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1792,13 +1792,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
});

// Wait for genesis block to be processed
{
WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
- kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip
...

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856178140)
With some manual testing I indeed get all three conditions from your comment to hit that assert. I'm just confused why.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2497346388)
@laanwj, Right! And `ps ax` in the VM looks suspiciously scarce: https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491003878 showing just `bash` and `03_test_script.sh`.

Another source of false positive could be if somebody from the outside initiates communication to the VM to which it responds. E.g. an outsider tries to connect to the VM to which it responds with an outbound packet e.g. TCP RST. At least that should be obvious from the error log, showing the incoming packet first
...

👍 rkrux approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2457786528)
re-ACK 1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3

Successful make and tests. I reviewed the range diff.

```
git range-diff 878b6c8...1ca5f7b
```

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1856221103)
The cleanup is still there so that other tests following it in the Python script are not impacted.

💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1856227125)
Outside the context of a snapshot, we only call `LoadChainTip()` if `if (!is_coinsview_empty(chainstate)) `:

https://github.com/bitcoin/bitcoin/blob/2638fdb4f934be96b7c798dbac38ea5ab8a6374a/src/node/chainstate.cpp#L147-L150

That explains why these three scenarios are different.

Still looking into where it _is_ set.

💬 martinus commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-2497416409)
@l0rinc I'm not working on it any more, feel free to take over!

✅ fanquake closed a pull request: "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile"
(https://github.com/bitcoin/bitcoin/pull/28226)

🤔 rkrux reviewed a pull request: "Add and use `satToBtc` and `btcToSat` util functions"
(https://github.com/bitcoin/bitcoin/pull/31356#pullrequestreview-2457837013)
Thanks for picking this up! Left few suggestions.
The PR can be split into 2 commits for each util function that will make it easier to process piece by piece.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856260835)
```diff
- def branch(prevout, initial_value, max_txs, tree_width=5, fee=0.00001 * COIN, _total_txs=None):
+ def branch(prevout, initial_value, max_txs, tree_width=5, fee=btcToSat(0.00001), _total_txs=None):
```

Seems to work.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856236488)
I see that `COIN` is defined in this file but not sure if this is the right file to put these functions in, maybe `util.py`?

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856293868)
Maybe accepting a string with internal string to decimal checking and conversion would also make it more flexible? The explicit `Decimal` conversion could be removed here then.

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856247723)
Maybe this utility function can become more useful if it accepts float too, thereby getting rid of explicit conversion while calling?

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856297065)
Doesn't `satToBtc` accept an `int`?

💬 rkrux commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1856237945)
Sorry for using camelCase in the issue description, snake_case is preferred in functional tests.