💬 achow101 commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2234439584)
ACK 734076c6de1781f957c8bc3bf7ed6951920cfcf6

🚀 achow101 merged a pull request: "Wallet: Add `max_tx_weight` to transaction funding options (take 2)"
(https://github.com/bitcoin/bitcoin/pull/29523)

👍 pablomartin4btc approved a pull request: "Rendering an amp characters in the wallet name for QMenu"
(https://github.com/bitcoin-core/gui/pull/828#pullrequestreview-2184202382)
tACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0

<details>
<summary>It corrects the bug in master.</summary>

- before


- after


</details>

💬 pablomartin4btc commented on pull request "Rendering an amp characters in the wallet name for QMenu":
(https://github.com/bitcoin-core/gui/pull/828#discussion_r1681843714)
I agree with @hebasto. Small nit:
```
// Menu items remove single &. Single & is shown when && are in
// the string. So replace & with &&.
```

💬 pablomartin4btc commented on pull request "Rendering an amp characters in the wallet name for QMenu":
(https://github.com/bitcoin-core/gui/pull/828#discussion_r1681847988)
In case the above suggestion is not taken, typo here:
```suggestion
// A single ampersand in the menu item's text sets a shortcut for this item.
```

👍 tdb3 approved a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444#pullrequestreview-2184314661)
re ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

Re-ran manual tests:
```
$ src/bitcoind -regtest -rest=1
$ src/bitcoin-cli -regtest createwallet test
$ src/bitcoin-cli -regtest -generate 101
$ src/bitcoin-cli -named sendtoaddress address=bcrt1qg2yks0c0slr8nklg30ts48lcq7yslcvaak7n9k amount=39 fee_rate=3
65203a75428597267c78b3781420cae8d753dcd0cf31d9b905bb904ca9431438
$ src/bitcoin-cli -generate 1
{
"address": "bcrt1q73g2xwe4ktc4zfyu8uaft9y8gj0skp7kxnuzvl",
"blocks": [
"5ce35
...

👍 tdb3 approved a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453#pullrequestreview-2184376970)
ACK faed5d3870fb32fb5278961ee23e38fd9ea6ca15

Test causes framework to be "non-shy"


💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682048700)
Split into two commits.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050138)
Done.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050222)
Done.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050453)
Removed.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682050701)
Updated the comment to @sipa's version.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682051928)
I haven't been able to come up with something that I think would be an improvement. I did split the second last commit up into two commits though. Maybe that will make it more clear when reviewing.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682053641)
You might be right here, but this function in particular is very heavily commented due to its importance. I think modifying the comment is better than just removing it.

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1682053666)
Hi @hodlinator do you know if there is something I can do to get this merged?

💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682113718)
Should be fixed (line is no longer touched)

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1682264742)
Don't know of a good way unfortunately. Maybe one of the other reviewers will react soon from our comment activity today.

Did a quick search for `HelpExampleRpc` now and there are a few other RPCs which have quite a few examples.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787)
Not sure the latest push is a good change. I don't think this should be replaced with a constructor in the future, but it should be removed completely.

A new function (for example `static std::optional<base_blob<N>> base_blob<N>::FromHex(std::string_view)` can be added that parses and then returns an `std::optional`. Similar to TryParseHex.

In any case, the doxygen comment here is probably the wrong place to track brainstorming issues.

If we really wanted to turn this into a constructor
...

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682324270)
Tried:
```diff
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index 2053a42d24..e8dfb03d18 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -119,6 +119,10 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality
BOOST_CHECK(uint160(R1S) == R1S);
BOOST_CHECK(uint160(ZeroS) == ZeroS);
BOOST_CHECK(uint160(OneS) == OneS);
+
+ uint256{0};
+ uint256{NULL};
+ uint256{nullptr};
}

BOOST_AUTO_TEST_C
...

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682344482)
> Does CI have a stricter warnings-as-errors policy?

Yeah, IIRC it should treat warnings as error.

Even if it didn't, the compile warning and `nullptr` in code would still have to pass the eyes of the developer and reviewers. Also, the CI would fail later on with a segfault anyway.

As for the others:

* `0` is intentional and used in code today, see:
```
const uint256 uint256::ZERO(0);
const uint256 uint256::ONE(1);
```

* NULL is forbidden by clang-tidy, see `modernize-use-null
...