💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901208893)
This is already the case. The transaction that is passed to `RelayTransaction` is added to `invs_to_send` using `wtxid/txid` based on `m_wtxid_relay` (that is part of the code you quoted already). For other transactions that may be in `inv_to_send` (e.g. ancestors and/or descendants), this only apply to Erlay, and Erlay enforces `WTXID_RELAY`

💬 edilmedeiros commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2568291604)
After many hours, I could make it work.

Add this to ~/.lldbinit to make it load a local .lldbinit file: `settings set target.load-cwd-lldbinit true`. This is a convenience so that I can keep everything related to Bitcoin Core in its repo.

In the Bitcoin Core repo folder, add a .lldbinit file with (mine is located at `/Users/jose.edil/2-development/bitcoin/bitcoin-core-dev`, adjust to your path):
```
settings set target.source-map /Users/jose.edil/2-development/bitcoin/bitcoin-core-dev/bu
...

💬 maflcko commented on pull request "[28.x] 28.1 backports (final?)":
(https://github.com/bitcoin/bitcoin/pull/31594#issuecomment-2568299441)
> Are these significant enough to warrant a rc3 or can we go straight to final?

Yes, seems fine in this special case, as the depends patch shouldn't affect the release bin and the other change is test-only. I went ahead and picked your commits from https://github.com/bitcoin/bitcoin/pull/31582

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1901215478)
The rationale for who this method is used is provided in `RelayTransaction` when the method is used

https://github.com/bitcoin/bitcoin/blob/804ccf030362fe5b232fe5c4213b77899a425f2b/src/net_processing.cpp#L2154-L2159

I think the docs give enough rationale as to what this is intended for without being excessively narrow as to what the use cases could be.

💬 brunoerg commented on issue "Fuzz: Runtime errors when running fuzz tests on MacOs":
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2568305291)
I have the same issue and I think other people also have reported it before, I simply ignore it.

💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2568305724)
> I am unable to crash the fuzz test when `setmocktime()` is removed from one of the targets.

Which target was it?

It could be that running the test from scratch doesn't quickly reach code where `now()` is called. Try running the target on a corpus of inputs:
```
FUZZ=load_external_block_file ./mocktimefuzzbuild/src/test/fuzz/fuzz ../qa-assets/fuzz_corpora/load_external_block_file -runs=1
```
You can find the corpora here: https://github.com/bitcoin-core/qa-assets

I think it's also
...

💬 marcofleon commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#discussion_r1901223667)
Thanks, yeah that makes more sense. This actually caught system time usage in `p2p_headers_presync` that wasn't crashing before. Because the `mocktime.count()` check is now in the function itself rather than at the end of the fuzz iteration.

Using that syntax didn't compile for me, but I believe what I just pushed should work.

🤔 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;
```