💬 nerd2ninja commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1796429893)
> This major Bitcoin protocol change will most likely result in services wanting to offer 0-conf to

0-conf is bad design philosophy from the start. Simply running a custom client with a different transaction relay policy or paying miners out of band breaks it. However, a 0-conf design with much better security guarantees has been designed. They're called "MAD transactions" you can read about them here: https://coinexplorers.com/general/mad-transactions-mutual-assured-destruction-transactions-
...

💬 brunoerg commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1796506821)
> Is there a reason not to cover more of DescriptorScriptPubKeyMan in this PR? e.g. DescriptorScriptPubKeyMan::SignTransaction or DescriptorScriptPubKeyMan::FillPSBT.

Not exactly, perhaps one of them may be very slow, let me test more functions and then I can add them all here, np.

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1796514184)
Concept ACK

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1384011959)
In fde0193e687ad50a01a191e14fb6c052b3534bc1: perhaps using f-strings in all similar cases?

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1384016700)
from https://github.com/sipa/asmap/pull/9/

```suggestion
print(
"# %i%s IPv4 addresses changed; %i%s IPv6 addresses changed"
% (
ipv4_changed,
"" if ipv4_changed == 0 else " (2^%.2f)" % math.log2(ipv4_changed),
ipv6_changed,
"" if ipv6_changed == 0 else " (2^%.2f)" % math.log2(ipv6_changed),
)
)
```

🤔 murchandamus reviewed a pull request: "validation: return more helpful results for reconsiderable fee failures and skipped transactions"
(https://github.com/bitcoin/bitcoin/pull/28785#pullrequestreview-1716255880)
crACK 0150e860863f95b18448e7b67f5db27017660670 with grain of salt:

I’m pretty new to reviewing mempool code:

- The introduction of an explicit class for `Wtxid` makes sense to me
- The introduction of `TX_RECONSIDERABLE` makes sense to me in the context of transactions being tested in the context of packages where they might be resubmitted with a higher resulting mining score
- It makes sense to me that some mempool acceptance tests would now fail with `TX_RECONSIDERABLE` and the ones th
...

💬 murchandamus commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1384000302)
After the name change, perhaps:

```suggestion
const bool is_reconsiderable{res.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE};
```

💬 murchandamus commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1384005669)
Perhaps:
```suggestion
const bool valid_or_reconsiderable{atmp_result.m_result_type == MempoolAcceptResult::ResultType::VALID ||
```

💬 brunoerg commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1384023104)
In fde0193e687ad50a01a191e14fb6c052b3534bc1: Is there any case of `state` not being `None` for any `load_file` usage?

💬 ismaelsadeeq commented on pull request "validation: return more helpful results for reconsiderable fee failures and skipped transactions":
(https://github.com/bitcoin/bitcoin/pull/28785#discussion_r1384025601)
PR description also needs to be updated to `TX_RECONSIDERABLE`

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1383974021)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts"

nit: When doing separate test cases like this, we should also use different wallets to prevent state from one test cases from effecting another. So this should be making its own wallets once again.

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1384017154)
In 28a7e883c2f9249213fddbe24948ec5bb90b0fad "wallet: track mempool conflicts"

It's not clear to me that this change has any effect.

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1384032166)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts"

It would be useful to have a comment that explains that 11 blocks are mined so that when they are invalidated, `tx2_conflict` does not get put back into the mempool.

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1383975379)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts"

nit: No need to make another wallet for the receiver, just use the default wallet.

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1384035046)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts"

This isn't actually necessary since `listunspent` has these fields already and the extra ones are just ignored.

💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1384024252)
In 003efbbe45079c4416810a025b2bc372559dff15 "test: Add tests for wallet mempool conflicts"

This can be compressed into one line using the `send` RPC.

```suggestion
raw_tx1 = alice.send(outputs=[{bob.getnewaddress(): 24.9999}], inputs=[unspents[0]], add_to_wallet=False)["hex"]
```

💬 achow101 commented on pull request "Test: followups to #27823":
(https://github.com/bitcoin/bitcoin/pull/28612#issuecomment-1796622219)
ACK 5ab6419f380cc0a8cde78b125f3eeee5fcba43ae

💬 achow101 commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1384073170)
In eeee4a5779ea20d859cff2e411ad46dd52384f1d "mempool: persist with XOR"

Closing and reopening the file once we have the xor seems kinda clunky. Why not have a function on `CAutoFile` that lets us set the xor key after the fact?

✅ achow101 closed an issue: "Extend feature_init.py file perturbations"
(https://github.com/bitcoin/bitcoin/issues/28603)

🚀 achow101 merged a pull request: "Test: followups to #27823"
(https://github.com/bitcoin/bitcoin/pull/28612)