💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1938574388)
Rebased.

I agree adding `start_height` could be useful. I grabbed the commits and set start height to the taproot activation height. Will leave it up to someone else to make it a PR.

Looks like I'm using it incorrectly though:

```
2024-02-12T12:17:04Z silentpaymentindex thread start
2024-02-12T12:17:04Z Syncing silentpaymentindex with block chain from height 0
2024-02-12T12:17:04Z ERROR: Commit: Failed to commit latest silentpaymentindex state
2024-02-12T12:17:04Z ERROR: UndoReadFro
...

💬 maflcko commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#issuecomment-1938577167)
```
wallet/scriptpubkeyman.cpp:2161:42: error: no member named 'GetEncryptionKey' in 'wallet::WalletStorage'
if (!Assume(DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, it->second.first, key))) {
~~~~~~~~~ ^

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1938581651)
Interesting, your first error seems related to `UndoData`, which start height shouldn't affect?

> The compiler is also warning about the block filter index:

Ah, this is a shadowing error, I can fix that. I think I'll go ahead and open a `start_height` index PR since this seems generally useful and I'll fix the error there. @fjahr adding you as co-author unless you have any objections?

💬 fjahr commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1938591797)
utACK da13cc2c8010cbccf37324cca4403cb346ecdd30

I still think my suggestions [here](https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1483608953) would be an improvement but are not strictly needed.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1938598929)
It's indeed a strange error. I nuked the existing silent payment index before running, so not sure what's going on here.

💬 epiccurious commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1486152101)
+1

👍 hebasto approved a pull request: "Update Node window title with the chain type"
(https://github.com/bitcoin-core/gui/pull/758#pullrequestreview-1875194209)
ACK 9d37886a3b6ce24f4a4a05193eb0d071655a8457, tested on Ubuntu 23.10.

💬 hebasto commented on pull request "Update Node window title with the chain type":
(https://github.com/bitcoin-core/gui/pull/758#discussion_r1486154180)
nit: Missed EOL.

✅ hebasto closed an issue: "Indicate network in the Node window title - similar to the main window"
(https://github.com/bitcoin-core/gui/issues/544)

🚀 hebasto merged a pull request: "Update Node window title with the chain type"
(https://github.com/bitcoin-core/gui/pull/758)

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