📝 MarcoFalke converted_to_draft a pull request: "ci: Start with clean env"
(https://github.com/bitcoin/bitcoin/pull/27976)
Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564529747

💬 hebasto commented on pull request "refactor: Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1647896184)
As a follow up, I'm pretty sure that all casts of `ArgsManager::ParseParameters`'s arguments in `test/argsman_tests.cpp` can be just dropped as well.

📝 furszy opened a pull request: "test: create wallet specific for test_locked_wallet case"
(https://github.com/bitcoin/bitcoin/pull/28139)
Coming from https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128.

Several test cases are relying on the node1 default wallet, which thanks to 'test_locked_wallet' is encrypted.
And can be only accessed within a specific timeframe (100ms), a duration internally set by the same test.

This situation introduces a potential race condition, where other tests must complete their operations within
the specified 100ms window to pass (otherwise the wallet gets re-locked and they fa
...

💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1647982643)
@ajtowns @mzumsande Thanks, so it appears there are actually two mostly-unrelated network buffer deadlock issues, and Martin's test is likely triggering both of them.

I agree with AJ that we should still fix the network side one, even if we can't (or don't want to address) the application buffer side one. Fixing the application buffer side one indeed seems a lot harder, and probably needs discussion beyond this PR.

It would be good to have a test for the network side one, without it also t
...

💬 glozow commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1272307586)
Question: why can't this constant stay in src/node/txreconciliation?

💬 glozow commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1272302469)
Lost the comment while moving this from net_processing.cpp.
I think it would be good to have doxygen comments on all of these members of `Options`.

```suggestion
/** Whether this node is running in -blocksonly mode */
bool ignore_incoming_txs{false};
```

🤔 glozow reviewed a pull request: "net processing, refactor: Decouple PeerManager from gArgs"
(https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1543550898)
concept ACK

💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1272315181)
Using string literals scattered across the code seems fragile. In this PR branch @ a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240, we already have two places:
- `sighashOptions` in `bitcoin-tx.cpp`, and
- `ParseSighash` in `core_read.cpp`

I'd avoid multiplying such cases. We'd better to refactor code (as a follow up) out into a dedicated structure/function.

💬 MarcoFalke commented on issue "p2p_getaddr_caching.py failure in TSan CI":
(https://github.com/bitcoin/bitcoin/issues/28133#issuecomment-1647988007)
https://cirrus-ci.com/task/6450543510421504?logs=ci#L3872 in Asan

💬 MarcoFalke commented on issue "p2p_getaddr_caching.py failure in TSan CI":
(https://github.com/bitcoin/bitcoin/issues/28133#issuecomment-1647990885)
cc @vasild 20b49460b35268db59f7efcb02736b0e31191a74

💬 TheCharlatan commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1272327549)
Moving it to the place where it is actually used seems like a good idea, no?

💬 hebasto commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1648006460)
Concept ACK.

💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#issuecomment-1648008840)
lgtm ACK c648bdbda21c7ae90c6b40e506ca4ed62b1dbb6c

🤔 furszy reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1543641018)
Left few comments. I think that we could go further in few places and check that the called functions are actually doing what they suppose to be doing.

For instance, all valid results must have a target below the sum of the selected inputs amounts. Also, waste on results with more difference between target and inputs sum should be greater than results with closer diff between target and inputs sum.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1272359281)
nit: To not have to do the transformation, could move all the `std::vector<COutput>` to `std::vector<std::shared_ptr<COutput>>`

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1272371310)
In d72c0926:

Should check the `Merge` outcome. e.g. the result target and weight need to be the sum of the two merged targets and weights. Moreover, if one result uses the effective value and the other one not, the `use_effective` field must be updated.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1272364737)
Couldn't we assert that BnB `GetChange()` is always 0?

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1272387005)
No idea where this came from. Fixed.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1272387694)
Fixed.

I've chosen not to introduce an enum, because it's not a very good with with the multiple bit error cases.

🤔 pinheadmz reviewed a pull request: "wallet: clarify replace fields in help output"
(https://github.com/bitcoin/bitcoin/pull/27782#pullrequestreview-1543745814)
concept ACK

Included a nit for better consistency with the messages. I'll also point out, regarding the original issue, that `gettransaction` also returns a `walletconflicts` field which is present for both senders and receivers.