💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722338360)
Ah, so it's just out of scope - I can work with that. :)

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2297461028)
The major change in the latest push is that I've broken out the changes to `CCrypter` + tests into several more commits (code improvement, de-Hungarianization rename, vector->span, separate XOnlyPubKey) to try to clarify what is actually happening. Could have sworn someone suggested it but having trouble finding the comment.

I've implemented `ScriptFromHex` in terms of a modified `ToScript` in a last attempt before [maflcko gets to veto it](https://github.com/bitcoin/bitcoin/pull/30377#discus
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722354071)
Unless someone finds a way to maintain symmetry using ranges or something, I'm keeping as-is.

💬 tdb3 commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1722354614)
Thanks for reviewing. Implemented

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722356732)
Settled for fixing this in 41d97d38bfbdacdf56e730c6b082f992e4f15ec1, before the scripted-diff happens.

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2297470012)
> I'm surprised at the lack of (significant) per-iteration time reduction for:
>
> * clusterlin: reduce computation of unnecessary pot sets (optimization)
> * clusterlin: avoid recomputing potential set on every split (optimization)
> * clusterlin: avoid jump ahead in unmodified pot sets (optimization)

I have dropped the 2nd and 3rd, due to inconclusive evidence that they help. The 1st one I have kept; my belief is that it in fact does not improve the worst case, but it is also has very l
...

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2297476666)
Thanks for your patience, re-reviewed everything from scratch - the focused commits help a lot.
ACK 80a596ecca5df9f471d9fbcd9fcd15ddd296cdca

💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1722364709)
I don't think this empty line should be removed.

💬 tdb3 commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#issuecomment-2297564821)
Thanks @danielabrozzoni and @brunoerg. Pushed an update to address comments, and clarified the log message.

📝 fjahr opened a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678)
I discovered that we don't write the best block to disk when trying to explain the behavior described here: https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882

In the context of that test, the behavior is confusing and I think it also shows that one of the already existing tests in `wallet_assumeutxo.py` doesn't actually test what it says. It only fails because the best block isn't written and actually, the height of the backup that is loaded is at the snapshot height during
...

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1722509473)
Ok, I see what you mean now. After chasing down some wrong leads I think I got it. The `rescan_height` is coming from the block locator stored on disk. This is a different value than `last_processed_block`. I found that there is not guaranteed that the latest block is written when a backup is created. Since the full database is included in the backup, there is indeed something there that would cause the wallet to only need to rescan after 299. So the correct behavior IMO would be that your case
...

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722535036)
I think you can resolve your own comment threads too 😉

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1722578637)
It's specifically documented that this is the behavior. Why shouldn't we be able to rely on it?

📝 tdb3 opened a pull request: "fix: handle invalid rpcbind port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679)
Previously, when an invalid port was specified in `-rpcbind`, `SplitHostPort()` return was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host).

This updates `HTTPBindAddresses()` to handle the invalid port before attempting the bind.

💬 tdb3 commented on pull request "fix: handle invalid rpcbind port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#issuecomment-2297833386)
Existing behavior:
```
src/bitcoind -rpcallowip=127.0.0.1 -rpcbind=127.0.0.1:18000 -rpcbind=[::1]:18001 -rpcbind=127.0.0.1:notaport -rpcbind=127.0.0.1:-18002 -rpcbind=[::1]:-18003
...
2024-08-20T02:13:49Z Command-line arg: rpcbind="127.0.0.1:18000"
2024-08-20T02:13:49Z Command-line arg: rpcbind="[::1]:18001"
2024-08-20T02:13:49Z Command-line arg: rpcbind="127.0.0.1:notaport"
2024-08-20T02:13:49Z Command-line arg: rpcbind="127.0.0.1:-18002"
2024-08-20T02:13:49Z Command-line arg: rpcbind="
...

💬 sipa commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2297842573)
This could cause a wallet (and its backup) to have a best block locator that's unknown to the block index, if a crash happens in between the backup and the next database flush. How do we behave when loading such a wallet file?

Other than that, Concept ACK.

💬 sipa commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#issuecomment-2297843609)
Concept ACK

🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2246852117)
See #30221. It fixes this in a more comprehensive way.
The comment I left is secondary.

💬 furszy commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1722605697)
If the node's chain is ahead of the wallet, you would be writing a locator in the future. Which would make the wallet skip blocks during the next rescan attempt.
Should lock the wallet mutex and obtain the wallet's best locator from the wallet's last processed block (`m_last_block_processed`).

💬 achow101 commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2297854473)
> This could cause a wallet (and its backup) to have a best block locator that's unknown to the block index, if a crash happens in between the backup and the next database flush. How do we behave when loading such a wallet file?

No issues, we just rescan from the most recent fork indicated by the locator. It's the same as loading a wallet when the node isn't synced yet.