💬 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 :)
(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.
(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
...
(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.
(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
...
(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
(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)
```
(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
...
(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);
```
(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.
(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?
(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
...
(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).
(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
...
(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.
(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
...
(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
(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.
...
(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
(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)
(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)