💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2113859955)
`SipHash` is decent (I tried several other candidates and most were slower), but not particularly fast (even a general SHA256-shani is in the same ball-park).
`RapidHash` is much simpler and, on short inputs, dramatically faster:
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 0.06 | 16,529,344,089.83 | 0.2% | 11.00 | `RapidHash_32b`
| 0.96 |
...

👋 l0rinc's pull request is ready for review: "blocks: force hash validations of blocks read from disk"
(https://github.com/bitcoin/bitcoin/pull/32638)

💬 sipa commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2114434674)
Perhaps I shouldn't have used the term "cryptographic"; I see it's referred to as "secure" instead of "cryptographic" in some places.

There are two properties that a (keyed) hash function needs to be cryptographic:
* Indistinguishable from a random function
* Have larger enough output to resist practical brute-force preimage or collision attacks.

RapidHash has neither. SipHash has the first (it was designed by cryptographers, and investigated cryptanalytically). HMAC-SHA256 has both.


...

💬 furszy commented on issue "Assertion failed: TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state) (wallet/wallet.cpp: AddToWallet: 1094)":
(https://github.com/bitcoin/bitcoin/issues/32625#issuecomment-2920129544)
> Is that the right PR? Those changes are not in the 29.x branch (I can add them to [#32589](https://github.com/bitcoin/bitcoin/pull/32589) if we want to backport).

Hmm yes, I thought we had included it on 29.x with [gui#864](https://github.com/bitcoin-core/gui/pull/864).. (these two combined cause the app to no longer open).

📝 ryanofsky opened a pull request: "Update libmultiprocess subtree to fix clang-tidy errors"
(https://github.com/bitcoin/bitcoin/pull/32641)
Includes:

- https://github.com/bitcoin-core/libmultiprocess/pull/165
- https://github.com/bitcoin-core/libmultiprocess/pull/173
- https://github.com/bitcoin-core/libmultiprocess/pull/172

These changes are needed to fix CI errors in #31802.

The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.
...

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2114520701)
(Sorry for the churn here)

I think we actually want to drop this `if` guard here and above, the CI job should fail right here if the path to `vsdevcmd.bat` breaks, so that the cause/fix is obvious.

💬 gmaxwell commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2920277088)
@instagibbs I believe the only cases that should be slower with SFL are either very small examples (where their time is irrelevant because its very low per tx compared to bigger clusters) and ones with huge numbers of dependencies. Beyond huge dependencies being generally unrepresentative they're also expensive to generate because every dependency needs a vin, they also tend to be so slow with both that I guess neither will run to completion in practice.

A better way to compare across imp
...

💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2920377815)
Tested this again with Warnet branch https://github.com/bitcoin-dev-project/warnet/pull/720, a feature that actually deploys a "local" Tor with its own directory authority into the cluster -- so I can deploy 20 regtest with onion addresses and monitor them all. Was able to catch to the private broadcast connections in the `netinfo` dashboard:

```
<-> type net serv v mping ping send recv txn blk hb addrp addrl age id address
out priv onion nwl2 2 0 0
...

💬 TheCharlatan commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2114618859)
I do think it would be fine if the binary format changes, which given the compressor is fairly likely. If we assume users in the worst case can always use the kernel library to deserialize the undo data in the future, that would guarantee that they will have a versioned decoder for it. Would this performance difference when using `ReadRawBlockUndo` be significant in the grand scheme when it comes to building an electrs index romanz?

👍 TheCharlatan approved a pull request: "Update libmultiprocess subtree to fix clang-tidy errors"
(https://github.com/bitcoin/bitcoin/pull/32641#pullrequestreview-2879410423)
ACK 9f6565488fc169898abc5f7ee620ce7a9a4e5e50

👍 ryanofsky approved a pull request: "thread-safety: fix annotations with REVERSE_LOCK"
(https://github.com/bitcoin/bitcoin/pull/32465#pullrequestreview-2879271077)
Code review ACK 96a341d4064132292621af404de908f01fbe3e2f. Nice changes modernizing the annotations and supporting better thread safety warnings.

@theuni I'd be happy to merge this since it now has 3 acks but I'm wondering what you think of the arguments davidgumberg pointed to in https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2112707324 against having a REVERSE_LOCK object at all?

I think those arguments are pretty compelling, and think maybe the ideal thing would be to close th
...

💬 ryanofsky commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2114541881)
In commit "thread-safety: add missing lock annotation" (aeea5f0ec112f9ec29da37806925e961c4998365)

Would it also make sense to add `AssertLockHeld(cs_main)` here? Developer notes say to "Combine annotations in function declarations with run-time asserts in function definitions"

💬 ryanofsky commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2114630987)
re: https://github.com/bitcoin/bitcoin/pull/32465#discussion_r2112707324

Thanks for finding the bugzilla link. Explanation there was very helpful.

I also do find the arguments against AutoUnlock pretty persuasive, and looking at REVERSE_LOCK calls in our codebase, it seems like they could pretty easily be dropped and replaced with plain unlock/lock calls.

Doesn't seem too important though. I think realistically if an exception is thrown, locking in the destructor should either be fine a
...

🤔 w0xlt reviewed a pull request: "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`"
(https://github.com/bitcoin/bitcoin/pull/32636#pullrequestreview-2879497779)
Approach ACK

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2114687612)
Don't worry, I'm reworking it to do more manifest checks anyway so changes at this point are fine.

Will look at this.

💬 achow101 commented on pull request "test: test MAX_SCRIPT_SIZE for block validity":
(https://github.com/bitcoin/bitcoin/pull/32304#issuecomment-2920598430)
ACK b1ea542ae651cec433910d8c727abc41f17a7678

🚀 achow101 merged a pull request: "test: test MAX_SCRIPT_SIZE for block validity"
(https://github.com/bitcoin/bitcoin/pull/32304)

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2920658650)
Rebased bda7626e987ce3d7287f72e6989dedde2c4ab666 -> d253fa9ebe4305118417a02aa72cc06810662ac6 ([`pr/libexec.5`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.5) -> [`pr/libexec.6`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.5-rebase..pr/libexec.6)) due to conflict with #28710

💬 achow101 commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#issuecomment-2920676506)
`PopulateWalletFromDB` inside of `CreateNew` should no longer be necessary.

It'd be nice if the load or create intention could be propagated down to the DB level as well. In `SQLiteDatabase::Open`, we pass the flags `SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE` which does both load and create. I think ideally it would be just `SQLITE_OPEN_READWRITE` for loading, and `SQLITE_OPEN_CREATE` for creating to further enforce that we expect a file to already exist for loading, and for no files to exis
...

💬 achow101 commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2114901397)
Since fees are not included when SFFO, I don't think this is reachable as should `available_balance` is always greater than `recipients_sum` in that case, so I don't think this needs any special SFFO treatment. Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return "Insufficent Funds", so this whole check could probably be guarded by an `if (m_subtract_fee_outputs)` to skip it when SFFO is on.