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

💬 Akashkuvar commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2162523467)
loadwallet "filename" ( load_on_startup )

💬 Akashkuvar commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2162523623)
loadwallet "filename" ( load_on_startup )

💬 Akashkuvar commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2162523915)
What price

💬 fanquake commented on pull request "doc: add release note for 29091 and 29165":
(https://github.com/bitcoin/bitcoin/pull/30261#discussion_r1636120485)
Yea. Will merge this now. If clang-16 happens, it can be updated, if not, it can be changed in the wiki.

🚀 fanquake merged a pull request: "doc: add release note for 29091 and 29165"
(https://github.com/bitcoin/bitcoin/pull/30261)

🚀 fanquake merged a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325)

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1636146833)
Regarding the visitor pattern, `CTxDestination` is a std::variant and we need the visitor to know to use `WitnessV1Taproot` when calculating the serialized size for both `GetSerializeSizeForRecipient` and `IsDust` when the `CTxDestination` is a `V0SilentPayments` destination. See https://github.com/bitcoin/bitcoin/pull/28201/commits/797e21c8c1bf393e668eeef8bb7ee9cae1e5e41e.

However, you're correct this isn't really clear in this PR, so I'll update the commit message to explain this.

Regard
...

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2162593232)
Should this be dropped:
https://github.com/bitcoin/bitcoin/blob/5ee6b76c69d51158c13f6ad9ea1b264372e58d4d/src/test/fuzz/fuzz.cpp#L75

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#issuecomment-2162616719)
Rebased to fix merge conflict and update commit message to better explain the motivation for using the visitor patter, per @S3RK 's feedback

💬 willcl-ark commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2162640621)
@emc99 you can run your node with `--prune=1024` which will prune old blocks and undo data from disk, where possible. This will mean that Bitcoin Core will take only 1024MB + UTXO set size (~12.5GB today) = ~13.5GB according to my pruned node.

```bash
will@ubuntu in ~/replay : 🐍 took 6s
₿ /home/will/bitcoin-builds/bitcoin-27.1rc1/bin/bitcoin-cli gettxoutsetinfo
{
"height": 847611,
"bestblock": "000000000000000000028f622110a7bcbb76ccf9090aa802927e9468840c30a5",
"txouts": 185213812
...

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1636218564)
Addressed in latest force push.

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#issuecomment-2162672165)
Force pushed to:
- address merged conflict from #28830
- address @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1624697758): inlined `DoWarning()` in commit "introduce and use the generalized node::Warnings interface" instead of in later commit "refactor: remove warnings globals" to avoid overhauling it twice.

💬 stickies-v commented on pull request "C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2162681582)
Force pushed to address merge conflict from #28830 and rebase on top of #30263