💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1635899839)
Fixed

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1635900277)
Fixed

💬 vasild commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1635902059)
Done

💬 vasild commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2162239639)
`77e34ded54...5f549c35d9`: fix typo in the comment: `s/socke/socket/`

💬 vasild commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1635907777)
Oh, it is usually either `s`, `sock` or `socket`. `socke` is something new, that is extra genuine ;-)

Fixed, invalidating 3 ACKs but should be trivial to re-review.

Thanks!

💬 vasild commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1635926463)
Alright, I did not realize this.
(can resolve this)

👍 theStack approved a pull request: "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30202#pullrequestreview-2112137538)
re-ACK 5f549c35d9a75c7019fe8a96963b988df5498eed 🧦

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2162284182)
> > This PR adds tests to cover two scenarios of loading a snapshot when the current chain tip is:
> >
> > * Not an ancestor of the snapshot block but has less work
> > * Not an ancestor or a descendant of the snapshot block and has more work
> >
> > In the second scenario, the snapshot block does not belong to the most-work chain anymore so I believe it covers this scenario too: `TODO: Valid snapshot file and snapshot block, but the block is not on the most-work chain`. Therefore I delet
...

💬 S3RK commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1635953882)
nit: I think the visitor pattern in `GetSerializeSizeForRecipient` and `IsDust` in this case makes the code less clear, but I could be missing something

💬 S3RK commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#issuecomment-2162300430)
Code Review ACK f004b1625bcb9b833f36af47b53675e3dd77135b

The PR doesn't modify the existing behavior, but prepares the code for Silent Payments (see #28201)
The code pushed down in the function doesn't affect the logic, because `txnew.vout` is not used between the old and the new location.

💬 vasild commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1635958429)
The frozen time will defeat `FUZZED_SOCKET_FAKE_LATENCY`:

https://github.com/bitcoin/bitcoin/blob/91e0beede2859dea987ba6db746e95dca0ceb024/src/test/fuzz/util/net.cpp#L229-L231

`RecvUntilTerminator()` and `SendComplete()` will never timeout. Is this acceptable trade off in order to have the fuzz test "more stable"? If it is "unstable" without freezing the time, does that mean that some of those methods actually timed out, resulting in a wider coverage? In this aspect, it seems that stable =
...

💬 S3RK commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1635958466)
p.s. I don't see how the callable will be applied to more than one destination, so in my mind it's just easier to inline the callable and drop the `std::visit`

👍 alfonsoromanz approved a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2112255189)
tACK cbc604f6a7739bf22a5d811b6366453860d59f4c

This PR successfully builds and passes the test when running `test/functional/feature_assumeutxo.py`.

Additionally, I tested this in the context of my PR ([#29996](https://github.com/bitcoin/bitcoin/pull/29996)) back when the code looked something like this:

```
def test_snapshot_in_a_divergent_chain(self, dump_output_path):
node = self.nodes[2]
# Rollback the chain down to START_HEIGHT
block_hash = node.getbl
...

💬 maflcko commented on pull request "doc: add release note for 29091 and 29165":
(https://github.com/bitcoin/bitcoin/pull/30261#discussion_r1636041925)
I can fix it up in the follow-up https://github.com/bitcoin/bitcoin/pull/30263

💬 emc99 commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2162431253)
But bitcoin core won't function without a copy of the blockchain, which is 550GiB in size

💬 maflcko commented on pull request "utils: add missing VecDeque include":
(https://github.com/bitcoin/bitcoin/pull/30268#issuecomment-2162450093)
ACK f51da34ec1a806d321a468691fa66082eef10ad9

💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2162468751)
> Would it be difficult to make the test deterministic?

Yes, the straightforward way would be to replace the `InsecureRand256()` with fixed hashes that don't result in transaction short IDs that collide. Less-straightforward but maybe better would be to keep using random hashes but check for a short ID collision and try again with a different random hash until the collision is gone -- or start with one random hash and change it in a way that guarantees the short ID (siphash) will be different
...

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2162473253)
> FWIW on Ubuntu 20.04 I'm using pre-compiled binaries from https://github.com/llvm/llvm-project/releases/tag/llvmorg-18.1.4 with no issues.

Yeah, seems fine for test systems. I didn't want to suggest it, because https://discourse.llvm.org/t/rfc-improve-binary-security/78121/56 is still WIP.

💬 maflcko commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#issuecomment-2162486216)
> `InsecureRand256`

I'd say longer them those global non-deterministic functions should go away anyway. Not something you need to do here, but if you want you can create a commit to accept a FastRandomContext reference as param to `BuildBlockTestCase`, then in each unit test, create a local FastRandomContext and bind it to a lambda named `BuildBlockTestCase`.

💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#issuecomment-2162517067)
> `user_error` was already logged at the right level through `GetNotifications().fatalError(user_error);`

Happy to review a follow-up for the two cases (https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633421463). For now I think it is risk-free and more consistent to log the fatal error twice in the `error` log category.