💬 l0rinc commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-2497162303)
@martinus, are you still working on this? If not, I'd like to take over, I've also noticed the same during profiling...

💬 hebasto commented on pull request "depends, refactor: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2497179590)
My Guix build:
```
x86_64
1166ebc7f2e39568ced2e7851d93e9521bc867a7b5e61b6416ef9623cc8b82c1 guix-build-d9c8aacce38a/output/aarch64-linux-gnu/SHA256SUMS.part
ec8f92b7a8f3046dcff7d66e20c4e2d719b96e9d9b36ecbcba20c701c39e96a2 guix-build-d9c8aacce38a/output/aarch64-linux-gnu/bitcoin-d9c8aacce38a-aarch64-linux-gnu-debug.tar.gz
27ae1f55b45872b62e9946ff57d5a5cda7f0be4d79409cc7c8b8f0ff5bf6917c guix-build-d9c8aacce38a/output/aarch64-linux-gnu/bitcoin-d9c8aacce38a-aarch64-linux-gnu.tar.gz
ce07cdb3f
...

👍 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?