🤔 mzumsande reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2521245377)
Thanks - your explanation makes sense - I did a bit of research on how much logic and I/O in constructors are considered to be "good style", and there doesn't really seem to be a clear consensus to me.

For me the main downside would be that some of the things the ctor does may be unexpected, hence below suggestion to add some documentation.

💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901166334)
`cache_sizes` is no longer used and can be removed from `CompleteChainstateInitialization()`

💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1901165977)
How about a comment here to document the non-trivial things this does (creating blockman, opening blocksdb, cleaning up old block files in case of pruning) and doesn't (opening coins db)? I think this would help understanding the sequence better.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901245990)
Calling `RegisterPeer` with the same `peer_id` multiple times will result in `ReconciliationRegisterResult::ALREADY_REGISTERED`.

The docs of both `PreRegisterPeer` and `RegisterPeer` specify that they should called only once.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901246821)
Added in [b84348a](https://github.com/bitcoin/bitcoin/pull/30116/commits/b84348a8fc93c86dcefb936f986ff2f1ced320b4)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901247541)
Addressed in 0e290996874200317ba4d3f1b3e12992b40db4d1

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901247923)
Addressed in a461a1204109762ea38b21d9b6c445e9b9ba570f

🤔 glozow reviewed a pull request: "BlockAssembler: return selected packages virtual size and fee"
(https://github.com/bitcoin/bitcoin/pull/30391#pullrequestreview-2528196008)
ACK 7de205d2c12a0e5c0dee68728b088b022f63e84d

📝 hebasto opened a pull request: "qa: Ensure consistent use of decimals instead of floats"
(https://github.com/bitcoin/bitcoin/pull/31595)
On NetBSD, with newer Python versions 3.12.8 and 3.13.1, many functional tests fail due to `float` numbers internal representation.

The typical error looks as follows:
```
$ python3.12 ./build/test/functional/feature_assumeutxo.py
...
2025-01-02T20:43:01.865000Z TestFramework (INFO): Submit a spending transaction for a snapshot chainstate coin to the mempool
2025-01-02T20:43:01.889000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/home/hebasto/dev/bit
...

💬 maflcko commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2568413121)
> many functional tests fail due to `float` numbers internal representation.
>
> A typical error looks as follows:

It would be good to explain this a bit better and provide a `--tracerpc` log, or similar.

💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901324742)
nit: The `Collision` here could be `LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, “…”)`.

🤔 ariard reviewed a pull request: "p2p: Fill reconciliation sets (Erlay) attempt 2"
(https://github.com/bitcoin/bitcoin/pull/30116#pullrequestreview-2528318804)
Reviewed up to 4b9f83a (“p2p: Makes transactions available…”), all the code changes.
I’ll check the test coverage.

💬 furszy commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1901360700)
nano nit:
``` c++
CKey key;
if (!GetPrivKey(/*pos=*/0, arg, key)) return false;
ret = EncodeSecret(key);
return true;
```

🤔 furszy reviewed a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2528398156)
looking good, only left few nits. No need to tackle them. Will test it properly in the coming days.

💬 furszy commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#discussion_r1901358754)
nit:
```c++
return key.IsValid();
```

🤔 ryanofsky reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2528244641)
Code review 7f34802fd726b259f7df6f605e1d488faf251127. I got pretty lost here and only reviewed about half of it so far, but overall I think the changes look good and make a lot of sense.

I left some suggestions below which I think could make code clearer, but they are not important so feel free to ignore any you disagree with.

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901278842)
In commit "Add DeriveTarget() to pow.h" (2f549b547207839ab666ac2b8b02811ff0990214)

I don't really like the approach here of duplicating the `CheckProofOfWorkImpl` code and having a comment saying a future refactor could deduplicate it. I think it could avoid confusion and bugs later on to just split the `CheckProofOfWorkImpl` function into two parts now in a way that makes it obvious behavior is not changing. Would suggest:

```diff
--- a/src/pow.cpp
+++ b/src/pow.cpp
@@ -143,7 +143,7 @@
...

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901345161)
In commit "rpc: gettarget" (0c56689e425684c0c5b2058c1a47f293ac7032e6)

It's not right to involve the `ser_uint256` function here, since converting the target `int` to a string should not involve bitcoin serialization. This can just use python string formatting `return f"{target:064x}"`

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901323476)
In commit "test: add nBits coverage" (ecb18b3405e4a61f872d03f667c167fb7c4a52cf)

I think it would be good to make this more descriptive. Maybe add "Compact representation of the block difficulty target, which the block hash interpreted as a little-endian number must not be greater than. This is hexadecimal string containing 32-bit number with the compact representation described in the arith_uint256 class of the difficulty target."

---

I also think it's really unfortunate the RPC is retu
...

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901357898)
In commit "test: add nBits coverage" (ecb18b3405e4a61f872d03f667c167fb7c4a52cf)

`return f"{nbits:08x}"` would be a more direct conversion from number to string and would make the python code more consistent with the c++ code.