💬 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)
💬 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)
(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-
...
(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])};
```
(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?
(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.
(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
(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.
(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.
(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)
(https://github.com/bitcoin/bitcoin/pull/30482)
👍 hebasto approved a pull request: "locks: introduce mutex for tx download, flush rejection filters once per tip change"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2195831402)
ACK c85accecafc20f6a6ae94bdf6cdd3ba9747218fd, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2195831402)
ACK c85accecafc20f6a6ae94bdf6cdd3ba9747218fd, I have reviewed the code and it looks OK.
💬 hebasto commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#issuecomment-2247080032)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30507#issuecomment-2247080032)
Concept ACK.
📝 TheCharlatan opened a pull request: "refactor: Add FlatFileSeq member variables in BlockManager"
(https://github.com/bitcoin/bitcoin/pull/30517)
Instead of constructing a new class every time a file operation is done, construct them once for each of the undo and block file when a new BlockManager is created.
In future, this might make it easier to introduce an abstract block store.
Historically, this was not easily possible prior to #27125.
(https://github.com/bitcoin/bitcoin/pull/30517)
Instead of constructing a new class every time a file operation is done, construct them once for each of the undo and block file when a new BlockManager is created.
In future, this might make it easier to introduce an abstract block store.
Historically, this was not easily possible prior to #27125.
💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689312472)
Good point.. the compile-time parsed bytes would go into a data section in the binary instead of the hex string literal, which should :tm: make the binary smaller. But when the `std::array` containers are initialized during runtime they will take up stack space, instead of the former `std::vector` taking up heap-space.
Heap is usually slower than the stack, but if the vector was just allocated in the local function, the data should be "hot" anyway.
Not sure how to elegantly achieve compile
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689312472)
Good point.. the compile-time parsed bytes would go into a data section in the binary instead of the hex string literal, which should :tm: make the binary smaller. But when the `std::array` containers are initialized during runtime they will take up stack space, instead of the former `std::vector` taking up heap-space.
Heap is usually slower than the stack, but if the vector was just allocated in the local function, the data should be "hot" anyway.
Not sure how to elegantly achieve compile
...
💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689327270)
> Heap is usually slower than the stack, but if the vector was just allocated in the local function, the data should be "hot" anyway.
Right, and I think there is no performance critical path.
> Not sure how to elegantly achieve compile time validation + runtime parsing.
Should be easy:
```cpp
struct ConstevalHexLiteral {
const char* const hex;
consteval ConstevalStringLiteral(const char* str) : hex{str} { assert(IsCHex(str)); }
consteval ConstevalStringLiteral(std::null
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689327270)
> Heap is usually slower than the stack, but if the vector was just allocated in the local function, the data should be "hot" anyway.
Right, and I think there is no performance critical path.
> Not sure how to elegantly achieve compile time validation + runtime parsing.
Should be easy:
```cpp
struct ConstevalHexLiteral {
const char* const hex;
consteval ConstevalStringLiteral(const char* str) : hex{str} { assert(IsCHex(str)); }
consteval ConstevalStringLiteral(std::null
...
💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689328166)
Yes, definitely different PR
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689328166)
Yes, definitely different PR