💬 ryanofsky commented on pull request "depends: Bump `libmultiprocess` for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30513#issuecomment-2246270392)
> Sorry for jumping the gun and causing extra churn with #30490. Hopefully this is the last bump for CMake :)

I probably should have asked about this when I reviewed #30490. I knew the cmake changes it included caused problems earlier, but I assumed the problems were fixed since then.

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2246282737)
Rebased 38c93e6a8e573dbdf3fc4d14be7483961e18a576 -> cf6d99bed842361fb06986780097fb80c9e879e4 ([`pr/indexy.51`](https://github.com/ryanofsky/bitcoin/commits/pr/indexy.51) -> [`pr/indexy.52`](https://github.com/ryanofsky/bitcoin/commits/pr/indexy.52), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/indexy.51-rebase..pr/indexy.52)) due to conflicts with #29671, #29776, #29867, #30058

> There's discussion in #29770 about whether to use some or all the things in this PR. Can you give it
...

🤔 Sjors reviewed a pull request: "index: Check all necessary block data is available before starting to sync"
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2194366605)
As a quick sanity check, I rebuilt `-coinstatsindex` and checked `gettxoutsetinfo` still gives me the same stats at block 840,000.

💬 Sjors commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r1688294883)
adabfbc23700056e5bd22b673c3078aa49ad83ea: it would be useful to have an example where `getblockfrompeer` _does_ work, i.e. an index that doesn't need undo data.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1688752764)
Bumped even further in https://github.com/bitcoin/bitcoin/pull/30513 :)

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1688758712)
Cross-posted in https://github.com/hebasto/bitcoin/issues/279 to make tracking easier.

📝 luisschwab opened a pull request: "rpc: add utxo's blockhash and number of confirmations to scantxoutset output"
(https://github.com/bitcoin/bitcoin/pull/30515)
This PR resolves #30478 by adding two fields to the `scantxoutset` RPC:
- blockhash: the blockhash that an UTXO was created
- confirmations: the number of confirmations an UTXO has relative to the chaintip.

The rationale for the first field is that a blockhash is a much more reliable identifier than the height:
> When using the scantxoutset RPC, the current behaviour is to show the block height of the UTXO. This is not optimal, as block height is ambiguous, especially in the case of a bloc
...

💬 luisschwab commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2246364773)
PR is up, reviews are appreciated.

🤔 hodlinator reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2194180912)
Concept ACK 9196cfe26da4784029500da482c6f7d55fca5ac2

### Concept

In https://github.com/bitcoin/bitcoin/pull/16545#discussion_r311440561 you say:

> Also this case really can and should be a compile error, not a runtime error.

That's along the lines of what I was going to bring up before reading the comments too. Hopefully this PR will take us further in that direction as you say.

### Commit messages

Both commits mention not changing application behavior 2 times each, seems a bit
...

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1688276366)
Typo: IntepretValue -> InterpretValue

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1688211499)
Please clarify that this is not a constructor of some type, but rather checking a boolean condition.
```suggestion
inline bool IsTypedArg(uint32_t flags)
```

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1688179608)
Ran ```make -j`nprocs` docs```:

Current:
After applying...
```diff
diff --git a/src/common/args.cpp b/src/common/args.cpp
index fd9efc265f..26eeabf4e3 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -109,7 +109,7 @@ KeyInfo InterpretKey(std::string key)
* helper methods can unambiguously determine original configuration strings
* from the JSON val
...

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1688743390)
Small adjustment to clarify that the specific value being set is not important.
```suggestion
error = strprintf("Can not negate -%s at the same time as setting a value ('%s').", key.name, *value);
```

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1688280437)
This is a bit opaque. Worth adding a comment such as:
`// Non-string values should always throw on this, even if untyped.`
? Not sure even that is correct though.

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1688231667)
Care to insert a comment about why you are avoiding the untyped version's `InterpretBool(*value)` here?

📝 fjahr opened a pull request: "Assumeutxo: Sanitize block height in metadata"
(https://github.com/bitcoin/bitcoin/pull/30516)
Fixes #30514 which has more context.

The block height in the assumeutxo metadata was implicitly cast to `int` which could cause it to be interpreted as signed.

The first commit shows how to reproduce the error shown in #30514 and how it could be fixed with a minimal change. The second commit adds an explicit sanitizer that will probably be the preferred approach. I can squash the commits but first I would like to hear conceptual feedback since #30514 suggested to remove the block height in
...

💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1688788130)
Having problems inferring the size of the underlying `std::array` from the `char`-array size using the constructor approach. Looks like a free function is where it's at for this one. (And maybe going back to raw `std::array` for the container if we fail compilation on whitespace/invalid input).

💬 fjahr commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2246412033)
Nice find, thanks for reporting!

I think it is worth keeping the block height around even if it seems completely redundant in the base case. The additional information can be helpful to us in certain failure cases, especially if we discuss issues like in #30288 seriously. So, I am suggesting to keep the block height and sanitize it in #30516.

> I presume it can also be reproduced with the integer sanitizer and:

Not quite, I have added the test to reproduce the failure in the first commi
...

💬 harding commented on pull request "Add imbued v3 based on template-matching":
(https://github.com/bitcoin/bitcoin/pull/29427#issuecomment-2246696006)
> the “must have exactly 2 330-satoshi outputs” i can just add another HTLC p2wsh 330 sats and your template is broken.

I believe @ariard is correct: anchor-style commitment transactions can additionally have 330-sat HTLC outputs.

💬 maflcko commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1689146902)
The fix isn't enough. The file may be completely untrusted and corrupt, so just because the block height is a signed 32-bit, but positive integer, doesn't mean that the height is correct.

You'll have to check that the block height corresponds to the block hash, otherwise I fail to see how it can be used without adding bugs or risks or confusion. However, if you do the check, you may as well discard the deserialized value and just use the value from the check.

So again, my recommendation wo
...