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

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2246924772)
`ecba2fbd20...e0bb306436`: rebase due to conflicts

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689162345)
I am a bit worried that putting the bytes directly onto the stack (or easily allowing the dev to do so) may lead to high stack usage in some code paths and stack overflow on some platforms that have tighter limits.

Maybe a better overall approach is to validate the hex string at compile time, but then parse into a (heap) vector at runtime?

I don't think there are any performance concerns where hex parsing is used right now, so doing the parsing at runtime or compile time shouldn't matter.
...

👍 maflcko approved a pull request: "rpc: add utxo's blockhash and number of confirmations to scantxoutset output"
(https://github.com/bitcoin/bitcoin/pull/30515#pullrequestreview-2195686882)
Left some style nits, some unrelated to your changes. Also, you can add a test to `test/functional/rpc_scantxoutset.py` for the new fields, if you want, but this should be fine either way.

ACK b86f4a66a5056a2975902e6e84e324aa6f920ebf

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1689170896)
unrelated nit: `coin.nHeight` is already a 31-bit unsigned integer, so the cast to `(int32_t)` doesn't change the value or the sign, and can be removed. (This will push the value as uint32_t instead, which is fine)

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1689166651)
nit: While touching this, you can also clarify the `{RPCResult::Type::NUM, "height", "The current block height (index)"` to say "The block height the scan was done at" (or similar), because the current block height may be different from the block height the scan was done at.

Same for `"bestblock"` (the next field)

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1689206283)
nit: According to https://eel.is/c++draft/conv.prom#5 `coin.nHeight` in this context will already be promoted to `int` (`int32_t` in Bitcoin Core). Also, `1` and `tip->nHeight` are `int`, so no need to cast to `int` to `int`.

```suggestion
unspent.pushKV("confirmations", tip->nHeight - coin.nHeight + 1);
```

If you do want to explicitly cast, my recommendation would be to use `int32_t{...}`, which is compile-time safe and print a compile error if the conversion wasn't value-
...

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1689168988)
Nit: Because you assume this always exists, may as well use a reference, rather than a pointer (which can be null)

```suggestion
const CBlockIndex& coinb_block{*(*active_chain)[coin.nHeight]};
```

or, if you want to belt-and-suspenders check against a nullptr deref (UB):

```suggestion
const CBlockIndex& coinb_block{*CHECK_NONFATAL((*active_chain)[coin.nHeight])};
```

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689233001)
Why not?

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689234283)
To avoid UB on a nullopt deref. But happy to remove it, if you think there is a benefit in removing it.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689235747)
The duality will be removed in a follow-up, when the deprecated on is removed

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689236483)
Maybe. Seems best to leave for the follow-up that does it.

💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2247053123)
> > I presume it can also be reproduced with the integer sanitizer and:
>
> Not quite

Can you clarify what you mean with "not quite"? Locally, if I compile with `undefined,integer,float-divide-by-zero` and run my diff with: `UBSAN_OPTIONS="suppressions=$PWD/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/feature_assumeutxo.py`, I get the same crash.

Happy to take a look, if you share your steps from a clean build.

👋 maflcko's pull request is ready for review: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482)