💬 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
🚀 fanquake merged a pull request: "depends: build expat with CMake"
(https://github.com/bitcoin/bitcoin/pull/29878)
(https://github.com/bitcoin/bitcoin/pull/29878)
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#issuecomment-2247173779)
`9c50b2fe44...a179b1cbf5`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29420#issuecomment-2247173779)
`9c50b2fe44...a179b1cbf5`: rebase due to conflicts
🚀 fanquake merged a pull request: "depends: Bump `libmultiprocess` for CMake fixes"
(https://github.com/bitcoin/bitcoin/pull/30513)
(https://github.com/bitcoin/bitcoin/pull/30513)
⚠️ KonradStaniec opened an issue: "Unexpected behaviour when using `sortedmulti_a` descriptor"
(https://github.com/bitcoin/bitcoin/issues/30518)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Currently running:
```
getdescriptorinfo "tr(50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0,and_v(v:pk(b6df9cc452123b137ae8ad15927ff78b7e4e010a97b8ef6732d6d9d692abd993),multi_a(1,0153089cb23a34755aeba2737cb2134add1708e09dca8ae128ccdb1f035f3197,572706d9cf6b553f179daaf22f456b8e31200f59f51d9dcef936cde61918220e)))"
```
will respond with success for such descriptor:
```
...
(https://github.com/bitcoin/bitcoin/issues/30518)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Currently running:
```
getdescriptorinfo "tr(50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0,and_v(v:pk(b6df9cc452123b137ae8ad15927ff78b7e4e010a97b8ef6732d6d9d692abd993),multi_a(1,0153089cb23a34755aeba2737cb2134add1708e09dca8ae128ccdb1f035f3197,572706d9cf6b553f179daaf22f456b8e31200f59f51d9dcef936cde61918220e)))"
```
will respond with success for such descriptor:
```
...
💬 dergoegge commented on pull request "refactor: Add FlatFileSeq member variables in BlockManager":
(https://github.com/bitcoin/bitcoin/pull/30517#issuecomment-2247197570)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30517#issuecomment-2247197570)
Concept ACK
💬 dergoegge commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689355519)
style nit: Indenting the body of the new scope would be nice
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689355519)
style nit: Indenting the body of the new scope would be nice
👍 dergoegge approved a pull request: "m_tx_download_mutex followups"
(https://github.com/bitcoin/bitcoin/pull/30507#pullrequestreview-2195972297)
ACK 1a0212c8e5c25fce2e0a2cab3113b25f2fbc38b7
(https://github.com/bitcoin/bitcoin/pull/30507#pullrequestreview-2195972297)
ACK 1a0212c8e5c25fce2e0a2cab3113b25f2fbc38b7
🤔 hodlinator reviewed a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2195931519)
Concept ACK aaaa66d348f874b8acd3a9f6208dd90dc322b16b
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2195931519)
Concept ACK aaaa66d348f874b8acd3a9f6208dd90dc322b16b
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689341052)
Not sure whether we should only do `FromHex` testing or do it in addition to `SetHexDeprecated` testing.
Sure, we want to remove `SetHexDeprecated` but it is unclear when or how easily it could be done, do you have an exploratory branch doing the removal?
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689341052)
Not sure whether we should only do `FromHex` testing or do it in addition to `SetHexDeprecated` testing.
Sure, we want to remove `SetHexDeprecated` but it is unclear when or how easily it could be done, do you have an exploratory branch doing the removal?
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689329336)
nit: Missed opportunity to convert to curly braces? :)
Same below for `COutPoint`.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689329336)
nit: Missed opportunity to convert to curly braces? :)
Same below for `COutPoint`.
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689354816)
Curious: Why introduce this namespace instead of making a protected method in `base_blob` as I did in my exploration (404c115ea0f53785a640818c2058ef0591c8c6ac)?
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689354816)
Curious: Why introduce this namespace instead of making a protected method in `base_blob` as I did in my exploration (404c115ea0f53785a640818c2058ef0591c8c6ac)?
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689361143)
Heh, may do if I retouch for other reasons.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689361143)
Heh, may do if I retouch for other reasons.
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689363198)
It is still tested in this test case (see above), and it other test cases (indirectly). Am I missing something, or is there other missing test coverage?
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689363198)
It is still tested in this test case (see above), and it other test cases (indirectly). Am I missing something, or is there other missing test coverage?