๐ theStack opened a pull request: "rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes`"
(https://github.com/bitcoin/bitcoin/pull/33065)
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640) to get rid of the remaining hardcoded output types in wallet RPC documentation [1]. Note that it can't be used in the [`createmultisig` RPC](https://github.com/bitcoin/bitcoin/blob/fc162299f0cc5b61bbb55736fe85e27b705f78cc/src/rpc/output_script.cpp#L100), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
[1] instances
...
(https://github.com/bitcoin/bitcoin/pull/33065)
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640) to get rid of the remaining hardcoded output types in wallet RPC documentation [1]. Note that it can't be used in the [`createmultisig` RPC](https://github.com/bitcoin/bitcoin/blob/fc162299f0cc5b61bbb55736fe85e27b705f78cc/src/rpc/output_script.cpp#L100), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
[1] instances
...
๐ค jonatack reviewed a pull request: "ci: Run unit test parallel with functional tests"
(https://github.com/bitcoin/bitcoin/pull/33000#pullrequestreview-3056247871)
I don't know how much variance we typically see in CI run times, but light lgtm ACK fab25c6f5803ae8699f068d089f1c6e4f1387743
```
$ /test_runner.py -h
--valgrind run nodes under the valgrind memory error detector: expect at least a ~10x slowdown. valgrind 3.14 or later required. Does not apply to previous release
binaries.
--bash-cmd-extra-tests BASH_CMD_EXTRA_TESTS
Run an arbitrary string as Bash command in parallel to the fu
...
(https://github.com/bitcoin/bitcoin/pull/33000#pullrequestreview-3056247871)
I don't know how much variance we typically see in CI run times, but light lgtm ACK fab25c6f5803ae8699f068d089f1c6e4f1387743
```
$ /test_runner.py -h
--valgrind run nodes under the valgrind memory error detector: expect at least a ~10x slowdown. valgrind 3.14 or later required. Does not apply to previous release
binaries.
--bash-cmd-extra-tests BASH_CMD_EXTRA_TESTS
Run an arbitrary string as Bash command in parallel to the fu
...
๐ค LarryRuane reviewed a pull request: "test: revive test verifying that `GetCoinsCacheSizeState` switches from OKโLARGEโCRITICAL"
(https://github.com/bitcoin/bitcoin/pull/33021#pullrequestreview-3056300082)
tACK b2d82668a57d4ab4eef310c25a2aebda18563677
I thoroughly reviewed and understand all code changes. This test perfectly tests `GetCoinsCacheSizeState()` and is much simpler and less fragile. (Also, I learned a few C++ tricks, very nice.)
I ran the modified test using the debugger (on Ubuntu), and it behaves as designed. The only very minor suggestion if you need to retouch is to consider adding a comment explaining why it's not necessary to verify that the loops didn't exceed `MAX_ATTEMPTS`
...
(https://github.com/bitcoin/bitcoin/pull/33021#pullrequestreview-3056300082)
tACK b2d82668a57d4ab4eef310c25a2aebda18563677
I thoroughly reviewed and understand all code changes. This test perfectly tests `GetCoinsCacheSizeState()` and is much simpler and less fragile. (Also, I learned a few C++ tricks, very nice.)
I ran the modified test using the debugger (on Ubuntu), and it behaves as designed. The only very minor suggestion if you need to retouch is to consider adding a comment explaining why it's not necessary to verify that the loops didn't exceed `MAX_ATTEMPTS`
...
๐ฌ glozow commented on pull request "p2p: stop special-casing witness-stripped error for unconfirmed transactions":
(https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-3119690432)
I ran the above patch for a while and added logging to collect some stats and see how much more redownloading weโd do. TLDR it seems like we can definitely go ahead with it.
Caveats: Traffic has been very low so I set my mempool expiry to 1 hour to try to induce more orphan-fetching. Iโm a default policy node so I donโt really expect to be relayed many transactions Iโll reject. There isnโt much opportunity to test reconsiderable rejections right now because of how low transaction traffic is.
...
(https://github.com/bitcoin/bitcoin/pull/32379#issuecomment-3119690432)
I ran the above patch for a while and added logging to collect some stats and see how much more redownloading weโd do. TLDR it seems like we can definitely go ahead with it.
Caveats: Traffic has been very low so I set my mempool expiry to 1 hour to try to induce more orphan-fetching. Iโm a default policy node so I donโt really expect to be relayed many transactions Iโll reject. There isnโt much opportunity to test reconsiderable rejections right now because of how low transaction traffic is.
...
๐ฌ Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231703938)
This is better, will implement.
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231703938)
This is better, will implement.
๐ฌ Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231710505)
Will implement, the `ScopedScheduler` is better imo and `std::shared_ptr` makes more sense over `std::unique_ptr`.
> With a shared pointer, we can safely and explicitly manage these dependencies without introducing coupling between the logger and the scheduler.
I think there will always be coupling between the two since `Reset` may call `LogPrintLevel_`?
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2231710505)
Will implement, the `ScopedScheduler` is better imo and `std::shared_ptr` makes more sense over `std::unique_ptr`.
> With a shared pointer, we can safely and explicitly manage these dependencies without introducing coupling between the logger and the scheduler.
I think there will always be coupling between the two since `Reset` may call `LogPrintLevel_`?
๐ murchandamus's pull request is ready for review: "Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)
(https://github.com/bitcoin/bitcoin/pull/33060)
๐ฌ glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3119906723)
I cherry picked https://github.com/monlovesmango/bitcoin/commit/174827285797794928cd87732e2587fea70c0157 and fixed some of the incorrect comments.
> [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates comments to reflect that non-reconsiderable announcements are sorted before reconsiderable announcements.
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3119906723)
I cherry picked https://github.com/monlovesmango/bitcoin/commit/174827285797794928cd87732e2587fea70c0157 and fixed some of the incorrect comments.
> [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates comments to reflect that non-reconsiderable announcements are sorted before reconsiderable announcements.
๐ฌ achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2231808946)
They can be imported with `importdescriptors`.
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2231808946)
They can be imported with `importdescriptors`.
๐ฌ ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2231837660)
Why is it safe to assume that the transaction doesn't already have a child here?
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2231837660)
Why is it safe to assume that the transaction doesn't already have a child here?
๐ฌ mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231847269)
added these and `serialize.h`, hope that'll do. (never got iwyu running locally and the tidy job didn't cover the new file for some reason).
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231847269)
added these and `serialize.h`, hope that'll do. (never got iwyu running locally and the tidy job didn't cover the new file for some reason).
๐ฌ mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231849239)
yes, I think the part with the "three items" is still correct, it talks about the values of the map.
I replaced the part about the key from blockfilterindex.cpp by a reference to db_key.h
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231849239)
yes, I think the part with the "three items" is still correct, it talks about the values of the map.
I replaced the part about the key from blockfilterindex.cpp by a reference to db_key.h
๐ฌ mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231851121)
Done - I added a comment to the file, using parts from `blockfilterindex.cpp`.
I agree that `db_key.h` is nicer, changed it to that.
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2231851121)
Done - I added a comment to the file, using parts from `blockfilterindex.cpp`.
I agree that `db_key.h` is nicer, changed it to that.
๐ฌ mzumsande commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3120065955)
[03b67a2](https://github.com/bitcoin/bitcoin/commit/03b67a27d013e55891df69b1aef9425574b88f5e) to [936bd87](https://github.com/bitcoin/bitcoin/commit/936bd874ac1581de532317741f5ab26e875935b3):
Addressed feedback by @fjahr.
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3120065955)
[03b67a2](https://github.com/bitcoin/bitcoin/commit/03b67a27d013e55891df69b1aef9425574b88f5e) to [936bd87](https://github.com/bitcoin/bitcoin/commit/936bd874ac1581de532317741f5ab26e875935b3):
Addressed feedback by @fjahr.
๐ฌ achow101 commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3120080986)
ACK c5c1960f9350d6315cadbdc95fface5f85f25806
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3120080986)
ACK c5c1960f9350d6315cadbdc95fface5f85f25806
๐ฌ achow101 commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3120084517)
ACK d89c6fa4a71810cdb28395d4609632e1b22249b3
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3120084517)
ACK d89c6fa4a71810cdb28395d4609632e1b22249b3
๐ฌ macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231890541)
Shouldn't the coin_type be different for regtest/signet? /352h/1h instead of /352h/0h
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231890541)
Shouldn't the coin_type be different for regtest/signet? /352h/1h instead of /352h/0h
๐ค macgyver13 reviewed a pull request: "Silent Payments: Receiving"
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3056616801)
working on backup / restore testing I noticed a few issues.
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3056616801)
working on backup / restore testing I noticed a few issues.
๐ฌ macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231883781)
I think this change is breaking the ability to import sp descriptor with private keys defined for scan and spend
This is the warning I receive when using importdescriptors:
"Not all private keys provided. Some wallet functionality may return unexpected errors"
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231883781)
I think this change is breaking the ability to import sp descriptor with private keys defined for scan and spend
This is the warning I receive when using importdescriptors:
"Not all private keys provided. Some wallet functionality may return unexpected errors"
๐ achow101 merged a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845)
(https://github.com/bitcoin/bitcoin/pull/32845)