✅ maflcko closed a pull request: "test: Extend stale_tip_peer_management test"
(https://github.com/bitcoin/bitcoin/pull/23352)

🚀 glozow merged a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353)

💬 glozow commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1929276821)
ACK e7fd70f4b6

🚀 glozow merged a pull request: "test: make v2transport arg in addconnection mandatory and few cleanups"
(https://github.com/bitcoin/bitcoin/pull/29356)

💬 sicelo7 commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1479590947)
Ye

💬 Sjors commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1479616697)
My initial worry when seeing this PR was that it would touch mainnet consensus code in order to support these parameters. But clearly it doesn't, which is nice.

💬 Sjors commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1929320537)
There was also the idea of dropping this number entirely: https://github.com/bitcoin/bitcoin/pull/13875#issuecomment-410472259

💬 maflcko commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1929346759)
> Though see also https://github.com/bitcoin/bitcoin/pull/13875#issuecomment-750715082

This is `nTx`, not `nChainTx`.

💬 epiccurious commented on pull request "fuzz: remove unused `args` and `context` from `FuzzedWallet`":
(https://github.com/bitcoin/bitcoin/pull/29388#issuecomment-1929380707)
utACK b14298c5bca395e5ed6a27fe1758c0d1f4b824ac

💬 maflcko commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1929388354)
For reference, I removed `nChainTx` from consensus in https://github.com/bitcoin/bitcoin/pull/29349. The next steps would be to add a member to `CChain` to account for the sum of `nTx`, and update it when `SetTip` is called. Then, update the RPC, wallet and estimate functions to derive the `nChainTx` they need from the `nChainTx` at the tip, walking the relevant portion of the chain.

This is possible, but I am not sure if it is worth it, because:

* It is more code overall.
* The RPC, wall
...

💬 epiccurious commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1479680894)
The previous is commit per git blame is [test: Speed up cache creation](https://github.com/bitcoin/bitcoin/commit/fa473303972b7dad600d949dc9b303d8136cb7e7#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76R486) dated May 17, 2019 by @maflcko.

`-disablewallet` arg was added earlier in [qa: Premine to deterministic address with -disablewallet](https://github.com/bitcoin/bitcoin/commit/faa669cbcd1fc799517b523b0f850e01b11bf40a#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd
...

🤔 josibake reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1864949072)
Looking good, and thanks for adding more tests!

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479646534)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):

```suggestion
const TransactionError err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true);
```

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479660495)
in "wallet: add maxfeerate wallet startup option" (https://github.com/bitcoin/bitcoin/pull/29278/commits/4165dc41289fd337e279f3986dfc1a84345c900e):

Instead of touching this line twice, what do you think about reordering the commits so that this commit is first and the changes to `BroadcastTransaction` come after it? This way, you avoid needing to first set `/*max_tx_feerate=*/CFeeRate(0)` only to remove it in this commit.

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479649957)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):

Don't we have a `DEFAULT_TRANSACTION_MAXFEERATE`? Wouldn't it be better to use that here instead of 0?

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479650353)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):

Same comment as above regarding defaults

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479651121)
in "node: BroadcastTransaction should also check tx fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/30bf6674b117cb320984ac66133a496f19166160):

Same comment regarding defaults vs 0

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479686164)
in "wallet: test: enforce -maxfeerate on wallet transactions" (https://github.com/bitcoin/bitcoin/commit/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):

What's the reason for changing the tests instead of adding a new test case for `maxfeerate`? Seems like we should be testing both rather than changing the old `maxtxfee` tests to now check `maxtxfeerate`.

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479684395)
in "wallet: test: enforce -maxfeerate on wallet transactions" (https://github.com/bitcoin/bitcoin/pull/29278/commits/eadc3a4500d6b7be263a2981602c20f6e5eb13a4):

This should be `MAX_FEE_RATE_EXCEEDED`, right?

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479669661)
in "wallet: add maxfeerate wallet startup option" (https://github.com/bitcoin/bitcoin/pull/29278/commits/4165dc41289fd337e279f3986dfc1a84345c900e):

Why are you removing the `-maxtxfee` test? Why not just add a new test case for `-maxfeerate`?

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1479691126)
in "wallet: rename m_default_max_tx_fee to m_max_tx_fee" (https://github.com/bitcoin/bitcoin/pull/29278/commits/a1497af6fc11009839ef86de1bf2da5cf99ad489):

Agree that this is way less confusing as a name, but probably still good to have a commit message explaining your reasoning. I'd also suggest moving this commit to the beginning, before any of the tests are changed and also add the refactor label to the commit message.