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

💬 pinheadmz commented on pull request "wallet: clarify replace fields in help output":
(https://github.com/bitcoin/bitcoin/pull/27782#discussion_r1272425544)
```suggestion
{RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx replaces another."},
```

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1648152222)


On July 24, 2023 12:41:10 PM GMT+03:00, Pavel Vasin ***@***.***> wrote:
>I queried the explorer like this
>```
>~ $ curl -s https://mempool.space/api/v1/block/0000000000000000000153159d7b95debfb0dadcd1040aaf9dbeb0025a1ddeac/audit-summary | jq .fullrbfTxs
>[
> "53cec64b52989c531550ac4606bedf1ff83d5bfd90efdc4006f122ac6b1b7643",
> "5bc64344c56f847e2d992fab241567075473eec9423776afadc187299352bce1",
> "64ec51dedd1404a775d590f530a01bbdc4239c8eb6d33ce4b9217ec2f0b8ddae"
>]
>```
>Only 4 of 6 mention
...

💬 fanquake commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#issuecomment-1648154754)
cc @ishaanam

💬 MarcoFalke commented on pull request "refactor: Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1648205184)
Thanks, addressed both comments by reviewers.