💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1486149819)
in "[node]: update BroadcastTransaction to check fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/eedbeccbdb274841fc2cf2c53931e58fbea50dfc):


If a `max_tx_fee` is set, and a `max_tx_feerate` is not set, `BroadcastTransaction` might hit the max_tx_fee limit, so its not quite correct to say that "if CFeeRate(0), accept any fee." What about something like the following:

```
* @param[in] max_tx_fee reject txs with fees higher than this (if 0, the fee is not checked)
*
...

🤔 josibake reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1875177902)
Looking good! I think moving https://github.com/bitcoin/bitcoin/pull/29278/commits/d83ecdde0dfdbf46806be1310ad63716e33edc96 to be the 3rd commit (right after introducing the new option) makes more sense as there are still at least a few spots where you return a `MAX_TX_FEE` error incorrectly, and then fix it up in a later commit.

Also, small typo in https://github.com/bitcoin/bitcoin/pull/29278/commits/eedbeccbdb274841fc2cf2c53931e58fbea50dfc:

s/differenciating/differentiating/

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1486143881)
in "[node]: update BroadcastTransaction to check fee rate limit" (https://github.com/bitcoin/bitcoin/pull/29278/commits/eedbeccbdb274841fc2cf2c53931e58fbea50dfc):

This should be `MAX_FEERATE_EXCEEDED`

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1486152509)
in "[node]: update BroadcastTransaction to check fee rate limit" (https://github.com/bitcoin/bitcoin/commit/eedbeccbdb274841fc2cf2c53931e58fbea50dfc):

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

💬 hebasto commented on pull request "Bugfix: Address broken things around Peers detail view":
(https://github.com/bitcoin-core/gui/pull/677#issuecomment-1938671029)
Why the first commit "net: Ensure CNode.cleanSubVer is always assigned before nVersion" is needed?

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1486097145)
In commit b8900adcd232bcb2b7475be19b5457c91052ab5a:
Can you either add a line about its removal in the commit message, or do it in a separate commit?

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1485612372)
There is one `wallet` too many here.

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1486106049)
In commit b8900adcd232bcb2b7475be19b5457c91052ab5a:
Need to slightly adjust the comment here, how about:
`* @return true if we successfully combined the transactions, false if they were not compatible`

💬 hebasto commented on pull request "getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1938694223)
Concept ACK. However, I still agree with @promag's https://github.com/bitcoin/bitcoin/issues/19161#issuecomment-667726216:
> GUI users can use the RPC console.

💬 willcl-ark commented on issue "Assertion chainman().ActiveChain()[height] fails on startup on memory-constrained system":
(https://github.com/bitcoin/bitcoin/issues/29422#issuecomment-1938696443)
I have not been able to reproduce this error using v26.0 binaries in a docker container limited to 1GB ram. At the same reported moment of your first crash I was only using ~550MB of ram:



<details>
<summary>Log excerpt</summary>

```log
2024-02-12T12:14:50Z Bitcoin Core version v26.0.0 (release build)
2024-02-12T12:14:50Z InitParameterInteraction: parameter interaction: -listen=0 -> sett
...

💬 russeree commented on pull request "redeclare nChainTx to use uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1938698865)
Could I please have just a bit more time to work on this?

Thanks, Reese

💬 sr-gi commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1486199775)
I may be used to the pattern due to rustlang, but I have no strong opinions about it

🤔 hebasto reviewed a pull request: "Dialog for allowing the user to choose the change output when bumping a tx"
(https://github.com/bitcoin-core/gui/pull/700#pullrequestreview-1875289512)
The first commit e1ea0ba0f2aa9b36df6b74b664087b236f174990 fails to compile:
```
CXX qt/libbitcoinqt_a-walletmodel.o
qt/walletmodel.cpp: In member function ‘bool WalletModel::bumpFee(uint256, uint256&)’:
qt/walletmodel.cpp:486:41: error: no matching function for call to ‘interfaces::Wallet::createBumpTransaction(uint256&, wallet::CCoinControl&, std::vector<bilingual_str>&, CAmount&, CAmount&, CMutableTransaction&)’
486 | if (!m_wallet->createBumpTransaction(hash, coin_control, e
...

💬 hebasto commented on pull request "Dialog for allowing the user to choose the change output when bumping a tx":
(https://github.com/bitcoin-core/gui/pull/700#discussion_r1486212254)
To make translators' life easier and to keep the context as large as possible, I suggest to use `QString::arg`. Also it will fix the `lint-qt-translation.py` warning.

💬 josibake commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#issuecomment-1938735308)
ACK https://github.com/bitcoin/bitcoin/pull/28987/commits/9a3c5c8697659e34d0476103af942a4615818f4e

looks good, also `RemoveTxs` is a much better name. Might be worth updating the description to mention the rename since the description ends up in the commit log.

💬 hebasto commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-1938735622)
@GBKS What do you think from a designer's point of view?

💬 hebasto commented on pull request "Check for private keys disabled before attempting unlock":
(https://github.com/bitcoin-core/gui/pull/773#issuecomment-1938750300)
> > Also, what is the expectation here, merge and backport this PR without backporting #28724? Because, after #28724, this isn't a problem anymore.
>
> 28724 is a much scarier change since it deletes things from the wallet database and I wasn't sure if people were comfortable with that. If we move forward with it, then that can supersede this PR.

I'm going to merge this to get this bugfix into 27.0 as https://github.com/bitcoin/bitcoin/pull/28724 remains untagged.

🤔 josibake reviewed a pull request: "rpc: Drop migratewallet experimental warning"
(https://github.com/bitcoin/bitcoin/pull/28037#pullrequestreview-1875330161)
ACK https://github.com/bitcoin/bitcoin/pull/28037/commits/f1684bb88a878eb3f54e945db39ea69b14256eef

Looks like all the known bugs have been fixed and most of the performance improvements have been merged. The only two outstanding performance improvements are #28987 and #26008, both of which seem close/RFM. Given that we are also including a warning that this process is expected to take longer, I think its safe to drop the warning even without #28987 and #26008 (obviously, better if we can also
...

💬 maflcko commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-1938757000)
Not sure. All info displayed on the About page is fixed details about the software itself, not about the current (test)chain. So modulating the icon or title bar seems confusing, but no strong opinion.

💬 Sjors commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#discussion_r1486244390)
Dropped.