👍 pinheadmz approved a pull request: "net: remove orphaned CSubNet::SanityCheck()"
(https://github.com/bitcoin/bitcoin/pull/27106)
ACK 30a3230e86dfd49c771432be6219841df5066eb4
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 30a3230e86dfd49c771432be6219841df5066eb4
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPudFYACgkQ5+KYS2KJ
yTrYnA/+LcTOT8/zvgcEeQ/nMhbO3LijgTmdio0OZS/dMc6SKdeAT/mDXv0ThjhB
+K7FuHY9fYO9sbXqPNkrbEfGR3iwqCNu5ElEAmvOMqVZIfkLHy//AX/uKNlUx2h7
1bICzt3PnnhaeGUWWxUeU+HFSpLikl6OHUYZLJfT+2VTz8rdMxhSY5iKSUEZNDKZ
J3D3B5nM
...
(https://github.com/bitcoin/bitcoin/pull/27106)
ACK 30a3230e86dfd49c771432be6219841df5066eb4
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 30a3230e86dfd49c771432be6219841df5066eb4
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPudFYACgkQ5+KYS2KJ
yTrYnA/+LcTOT8/zvgcEeQ/nMhbO3LijgTmdio0OZS/dMc6SKdeAT/mDXv0ThjhB
+K7FuHY9fYO9sbXqPNkrbEfGR3iwqCNu5ElEAmvOMqVZIfkLHy//AX/uKNlUx2h7
1bICzt3PnnhaeGUWWxUeU+HFSpLikl6OHUYZLJfT+2VTz8rdMxhSY5iKSUEZNDKZ
J3D3B5nM
...
💬 jonatack commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1433537441)
re-ACK b5eba9ad001f1035dd641bc5880cb6bb53a8b07f
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1433537441)
re-ACK b5eba9ad001f1035dd641bc5880cb6bb53a8b07f
💬 achow101 commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1108881969)
I've updated the passphrase handling to match what #27068 does
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1108881969)
I've updated the passphrase handling to match what #27068 does
💬 achow101 commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1108883010)
> It would be useful to also test the behavior of the wallet encryption post-migration (eg. that migratewallet re-locks the wallet).
Added a check for that.
> A test for when the passphrase is provided but is incorrect could also be added here.
Done
> A test for when the wallet name is provided but is incorrect could also be added.
There's a test for that already in `test_unloaded`.
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1108883010)
> It would be useful to also test the behavior of the wallet encryption post-migration (eg. that migratewallet re-locks the wallet).
Added a check for that.
> A test for when the passphrase is provided but is incorrect could also be added here.
Done
> A test for when the wallet name is provided but is incorrect could also be added.
There's a test for that already in `test_unloaded`.
💬 achow101 commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1108884105)
Fixed
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1108884105)
Fixed
💬 achow101 commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#issuecomment-1433550228)
> > Wallet file verification failed. Failed to load database path '/tmp/bitcoin_func_test_w3iehdpt/node0/regtest/wallets/wrong_wallet_name'. Path does not exist. (-4)
>
> It would make more sense to throw a simpler error from `migratewallet` (as opposed to from `MakeDatabase`).
This is the same error that we currently give for `loadwallet`. I'm going to leave it as is so these are consistent with each other, but I agree it could be improved.
(https://github.com/bitcoin/bitcoin/pull/26595#issuecomment-1433550228)
> > Wallet file verification failed. Failed to load database path '/tmp/bitcoin_func_test_w3iehdpt/node0/regtest/wallets/wrong_wallet_name'. Path does not exist. (-4)
>
> It would make more sense to throw a simpler error from `migratewallet` (as opposed to from `MakeDatabase`).
This is the same error that we currently give for `loadwallet`. I'm going to leave it as is so these are consistent with each other, but I agree it could be improved.
🚀 achow101 merged a pull request: "New `outputs` argument for `bumpfee`/`psbtbumpfee`"
(https://github.com/bitcoin/bitcoin/pull/25344)
(https://github.com/bitcoin/bitcoin/pull/25344)
💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1433568160)
re: https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1294756069
Thanks for the review! I got rid of the unused test file and changed the `if` formatting as suggested
---
Updated eb50fcd6859d1730663159995e8477f6d892e7f4 -> 501ef88b9412b0d924abf32cf2de7fbcbbb69b8d ([`pr/bresult2.28`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.28) -> [`pr/bresult2.29`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.29), [compare](https://github.com/ryanofsky/bitcoin/c
...
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1433568160)
re: https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1294756069
Thanks for the review! I got rid of the unused test file and changed the `if` formatting as suggested
---
Updated eb50fcd6859d1730663159995e8477f6d892e7f4 -> 501ef88b9412b0d924abf32cf2de7fbcbbb69b8d ([`pr/bresult2.28`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.28) -> [`pr/bresult2.29`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.29), [compare](https://github.com/ryanofsky/bitcoin/c
...
💬 1440000bytes commented on pull request "New `outputs` argument for `bumpfee`/`psbtbumpfee`":
(https://github.com/bitcoin/bitcoin/pull/25344#issuecomment-1433572881)
Finally. Thanks @rodentrabies this would improve UX.
(https://github.com/bitcoin/bitcoin/pull/25344#issuecomment-1433572881)
Finally. Thanks @rodentrabies this would improve UX.
💬 martinus commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1108890317)
In `BOOST_AUTO_TEST_CASE(set)` there's another `std::vector<CPubKey> keys;` without reserve, I wonder why clang-tidy doesn't see these
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1108890317)
In `BOOST_AUTO_TEST_CASE(set)` there's another `std::vector<CPubKey> keys;` without reserve, I wonder why clang-tidy doesn't see these
💬 martinus commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1108879800)
nit: could be 10, 5 more are added :)
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1108879800)
nit: could be 10, 5 more are added :)
💬 ajtowns commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1106907468)
Rather than `bool is20`, `enum JSONVersion { JSON_1_BTC, JSON_2_0 }` might be clearer?
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1106907468)
Rather than `bool is20`, `enum JSONVersion { JSON_1_BTC, JSON_2_0 }` might be clearer?
💬 RandyMcMillan commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1433649698)
applying clang-formatting:
per `src/.clang-format`
may be useful
https://github.com/RandyMcMillan/bitcoin/commit/99e3dc536dca94dffd703e77572f6dca91f4bb26
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1433649698)
applying clang-formatting:
per `src/.clang-format`
may be useful
https://github.com/RandyMcMillan/bitcoin/commit/99e3dc536dca94dffd703e77572f6dca91f4bb26
📝 MarcoFalke converted_to_draft a pull request: "ci: Add copyright header check"
(https://github.com/bitcoin/bitcoin/pull/27100)
Currently some files have a copyright header and some don't. There is no check for this.
Not sure for which files we want to enforce copyright headers, if any at all. However if we do it for any files, it should be enforced by CI. See also the section "Copyright" in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#copyright
So implement a check for this and add a config file to the `test/lint/` directory.
Can be tested by emptying the config file and calling `(cd test/lint
...
(https://github.com/bitcoin/bitcoin/pull/27100)
Currently some files have a copyright header and some don't. There is no check for this.
Not sure for which files we want to enforce copyright headers, if any at all. However if we do it for any files, it should be enforced by CI. See also the section "Copyright" in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#copyright
So implement a check for this and add a config file to the `test/lint/` directory.
Can be tested by emptying the config file and calling `(cd test/lint
...
📝 brunoerg opened a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114)
Revives #17167. It allows whitelisting outgoing connections.
I previously revived it with #26441, however, some reviewers told me it's better to keep them splitted to facilitate reviews and probably having this one merged first since we need it to improve functional tests.
(https://github.com/bitcoin/bitcoin/pull/27114)
Revives #17167. It allows whitelisting outgoing connections.
I previously revived it with #26441, however, some reviewers told me it's better to keep them splitted to facilitate reviews and probably having this one merged first since we need it to improve functional tests.
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1433677572)
cc: @furszy @theStack
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1433677572)
cc: @furszy @theStack
📝 brunoerg converted_to_draft a pull request: "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound"
(https://github.com/bitcoin/bitcoin/pull/26441)
Built this PR on top of #17167 (that's been closed due to inactivity but had some Concept ACK). So, it allows whitelisting outbound peers.
This PR adds a new RPC `addpermissionflags` to be able to set up permission flags -`whitelist` thru RPC, so we don't need to restart our node if we want to add new flags.
E.g.
```sh
$ ./src/bitcoin-cli addpermissionflags ["noban", "mempool", "in", "out"] "127.0.0.1"
```
(https://github.com/bitcoin/bitcoin/pull/26441)
Built this PR on top of #17167 (that's been closed due to inactivity but had some Concept ACK). So, it allows whitelisting outbound peers.
This PR adds a new RPC `addpermissionflags` to be able to set up permission flags -`whitelist` thru RPC, so we don't need to restart our node if we want to add new flags.
E.g.
```sh
$ ./src/bitcoin-cli addpermissionflags ["noban", "mempool", "in", "out"] "127.0.0.1"
```
💬 brunoerg commented on pull request "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound":
(https://github.com/bitcoin/bitcoin/pull/26441#issuecomment-1433679228)
Converted it to draft to address suggestions and re-build it on top of #27114.
(https://github.com/bitcoin/bitcoin/pull/26441#issuecomment-1433679228)
Converted it to draft to address suggestions and re-build it on top of #27114.
💬 brunoerg commented on pull request "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound":
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1108995297)
Done in #27114
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1108995297)
Done in #27114
💬 brunoerg commented on pull request "rpc, p2p: add `addpermissionflags` RPC and allow whitelisting outbound":
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1108995480)
Done in #27114
(https://github.com/bitcoin/bitcoin/pull/26441#discussion_r1108995480)
Done in #27114