Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299144625)
Added extra text to the help.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299145052)
Renamed to `funder`.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299145315)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299145463)
Done
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2299146384)
Done, however there should not be an expectation that the topup will always work as it does not work for wallets that are encrypted.
🚀 achow101 merged a pull request: "Update libmultiprocess subtree to fix build issues"
(https://github.com/bitcoin/bitcoin/pull/33241)
👍 l0rinc approved a pull request: "Revert compact block cache inefficiencies"
(https://github.com/bitcoin/bitcoin/pull/33253#pullrequestreview-3152574449)
Concept ACK, excellent find.

I redid the benchmarks, got more modest changes with GCC and Clang:

<details>
<summary>C++ compiler .......................... GNU 14.2.0</summary>

```bash
echo "> C++ compiler .......................... GNU $(gcc -dumpfullversion)" && rm -rf build && cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ >/dev/null 2>&1 && cmake --build build -j$(nproc) >/dev/null 2>&1 && build/bin/bench_bitcoin -filter
...
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298811715)
nit: newline at the very end is customary for the linter and formatter to not complain
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298827191)
nit: direct iteration would be slightly simpler:
```suggestion
for (auto& tx : refs) {
AddTx(tx, /*fee=*/tx->vout[0].nValue, pool);
}
```
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298808174)
nit: `std::ranges::shuffle` might be simpler:
```suggestion
std::ranges::shuffle(refs, rng);
```
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298810753)
we could consume the result to make sure the benchmark isn't optimized away
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298820059)
alternatively this may be slightly more descriptive than `11` and it could even be used to init the sigspam:
```suggestion
FastRandomContext rng{/*fDeterministic=*/true};
auto sigspam{rng.randbytes<200>()};
```
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298807140)
nit: we might as well reserve `50'000` here
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298886174)
I redid the a8203e94123b6ea6e4f4a6320e3ad20457f44a28 cherry-pick, this line and the top comment are new additions (+ some conflicts were resolved slightly differently to how I have, but the result seems correct to me)
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2298873346)
nit:
```suggestion
BenchCBHAST cmpctblock{rng, /*txs=*/3'000};
```
💬 l0rinc commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2299068765)
as mentioned by @instagibbs and @achow101, we could add two benches here, for empty extra and with some (partially overlapping) one, something like:
```C++
std::vector<std::pair<Wtxid, CTransactionRef>> extra_txn;
// ...
extra_txn.reserve(extra_tx_count);
for (size_t i{0}; i < extra_tx_count; ++i) {
size_t index{i < (extra_tx_count / 2) ? mempool_size + i : mempool_size - i}; // some overlap with mempool
auto tx{CreateTestTransaction(sigspam
...
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299170764)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

This seems unnecessary, it's the same thing `CRPCConvertParam`.
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299177873)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

Instead of parsing the argument directly, we should be using either `Parse` or `ArgToUniValue`
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299171197)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

nit: these should use the new naming style.
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2299173151)
In a4f2144facb5d4dc2f749494ea65744fffcb628b "rpc: Handle -named argument parsing where '=' character is used"

I find the name 'next_...` to be confusing since this is really referring to the position of the current argument, not the next argument.