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

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

💬 hebasto commented on pull request "m_tx_download_mutex followups":
(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.

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

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

💬 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

🚀 fanquake merged a pull request: "depends: build expat with CMake"
(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

🚀 fanquake merged a pull request: "depends: Bump `libmultiprocess` for CMake fixes"
(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:
```

...

💬 dergoegge commented on pull request "refactor: Add FlatFileSeq member variables in BlockManager":
(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