💬 0xB10C commented on pull request "build: remove systemtap variadic patch":
(https://github.com/bitcoin/bitcoin/pull/29181#issuecomment-1877675266)
ACK. That patch isn't needed anymore.
(https://github.com/bitcoin/bitcoin/pull/29181#issuecomment-1877675266)
ACK. That patch isn't needed anymore.
💬 brunoerg commented on pull request "fuzz: set `nMaxOutboundLimit` in connman target":
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1877676278)
friendly ping: @dergoegge
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1877676278)
friendly ping: @dergoegge
💬 dzyphr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877684914)
> > > Concept ACK
> > > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
> >
> >
> > As long as miners continue to mine these transa
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1877684914)
> > > Concept ACK
> > > The transactions targeted by this pull-req. are a very significant source of prohibitive cost for regular node operators. It is very unlikely that regular node operators will give up mitigating that source of operative cost. Regular policy rule for these transactions would encourage the economical resource usage of mempools - not harming any miners - neither changing any fee estimation already on the field.
> >
> >
> > As long as miners continue to mine these transa
...
💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-1877688376)
> 1 - rename "void(KEY)" to "unused(KEY)"
> 3 - add logic to `importdescriptors` to disallow importing an unused(KEY) if KEY is used by another descriptor.
Done these. I've also added a further restriction that unused() cannot be import to a wallet without private keys. It isn't useful in such wallets so I think it makes sense to disallow their import.
> 2 - add logic to `importdescriptors` and `generatewalletdescriptor` to delete any unused(KEY) descriptor as soon as the KEY which it r
...
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-1877688376)
> 1 - rename "void(KEY)" to "unused(KEY)"
> 3 - add logic to `importdescriptors` to disallow importing an unused(KEY) if KEY is used by another descriptor.
Done these. I've also added a further restriction that unused() cannot be import to a wallet without private keys. It isn't useful in such wallets so I think it makes sense to disallow their import.
> 2 - add logic to `importdescriptors` and `generatewalletdescriptor` to delete any unused(KEY) descriptor as soon as the KEY which it r
...
💬 hebasto commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#issuecomment-1877692626)
Concept ACK.
It makes the `libbitcoinkernel` code equivalent for both internal and external usage.
(https://github.com/bitcoin/bitcoin/pull/29180#issuecomment-1877692626)
Concept ACK.
It makes the `libbitcoinkernel` code equivalent for both internal and external usage.
🤔 luke-jr requested changes to a pull request: "wallet, rpc: add BIP44 `account` in `createwallet`"
(https://github.com/bitcoin/bitcoin/pull/29129#pullrequestreview-1804913032)
Beyond wallet_name, none of the positional parameters make sense as positional, and that goes for this new one too. Migrating this to an options object should probably be done sooner rather than after making the problem worse
(https://github.com/bitcoin/bitcoin/pull/29129#pullrequestreview-1804913032)
Beyond wallet_name, none of the positional parameters make sense as positional, and that goes for this new one too. Migrating this to an options object should probably be done sooner rather than after making the problem worse
🤔 hebasto reviewed a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1804912723)
In the master branch, the following code: https://github.com/bitcoin/bitcoin/blob/dcfb9ee9f6c8ce1f8c1e5dffdb207e55098ee9c5/src/crypto/sha256.cpp#L635-L640 compiles regardless of the `BUILD_BITCOIN_INTERNAL` macro. Now it is gated with the `DISABLE_OPTIMIZED_SHA256` one. Is it intentional?
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1804912723)
In the master branch, the following code: https://github.com/bitcoin/bitcoin/blob/dcfb9ee9f6c8ce1f8c1e5dffdb207e55098ee9c5/src/crypto/sha256.cpp#L635-L640 compiles regardless of the `BUILD_BITCOIN_INTERNAL` macro. Now it is gated with the `DISABLE_OPTIMIZED_SHA256` one. Is it intentional?
💬 hebasto commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#discussion_r1442214009)
nit:
```suggestion
#endif // DISABLE_OPTIMIZED_SHA256
```
(https://github.com/bitcoin/bitcoin/pull/29180#discussion_r1442214009)
nit:
```suggestion
#endif // DISABLE_OPTIMIZED_SHA256
```
💬 darosior commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1442215861)
Interesting. I was aware of `open_memstream` but not `memfd_create`. It's slower but could actually be helpful to get rid of the `128 MiB` allocs and make it possible to reindex.
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1442215861)
Interesting. I was aware of `open_memstream` but not `memfd_create`. It's slower but could actually be helpful to get rid of the `128 MiB` allocs and make it possible to reindex.
💬 luke-jr commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1877715274)
Concept NACK on the top commit: having each one in a separate file is nice.
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1877715274)
Concept NACK on the top commit: having each one in a separate file is nice.
⚠️ furszy opened an issue: "Prune Node Rescan Project Tracking"
(https://github.com/bitcoin/bitcoin/issues/29183)
This issue will be edited frequently to reflect the current status of the project.
#### What should I review now?
👇 👇 👇 👇 👇 👇
#28170 and #28955
☝️ ☝️ ☝️ ☝️ ☝️ ☝️
The goal of this project is to enable complete blockchain rescan capability on prune nodes, allowing these storage-limited nodes to import new wallet descriptors and update existing ones at any time, as well as loading external wallets with a birth-time older than 48 hours. This, in turn, enables the monitoring of
...
(https://github.com/bitcoin/bitcoin/issues/29183)
This issue will be edited frequently to reflect the current status of the project.
#### What should I review now?
👇 👇 👇 👇 👇 👇
#28170 and #28955
☝️ ☝️ ☝️ ☝️ ☝️ ☝️
The goal of this project is to enable complete blockchain rescan capability on prune nodes, allowing these storage-limited nodes to import new wallet descriptors and update existing ones at any time, as well as loading external wallets with a birth-time older than 48 hours. This, in turn, enables the monitoring of
...
💬 achow101 commented on pull request "wallet, rpc: add BIP44 `account` in `createwallet`":
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1877735190)
> You mean have a specific RPC for external signers or extending `createwalletdescriptor`?
Unsure of the final design, but something like `createwalletdescriptor` for external signers seems like it would be useful, and having an account argument in that would probably also make sense. Actually, that might make sense for `createwalletdescriptor` itself.
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1877735190)
> You mean have a specific RPC for external signers or extending `createwalletdescriptor`?
Unsure of the final design, but something like `createwalletdescriptor` for external signers seems like it would be useful, and having an account argument in that would probably also make sense. Actually, that might make sense for `createwalletdescriptor` itself.
💬 hebasto commented on pull request "util: remove Boost posix_time usage from GetTime*":
(https://github.com/bitcoin/bitcoin/pull/21110#issuecomment-1877739664)
@fanquake
> I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`...
> I'm not sure that waiting the many years until the project will actually be able to use C++20...
As `std::chrono::parse` is available now, do you mind submitting your follow up? If not, I can do it :)
(https://github.com/bitcoin/bitcoin/pull/21110#issuecomment-1877739664)
@fanquake
> I have a followup that should remove the last of our `boost:posix_time` usage in `ParseISO8601DateTime`...
> I'm not sure that waiting the many years until the project will actually be able to use C++20...
As `std::chrono::parse` is available now, do you mind submitting your follow up? If not, I can do it :)
💬 sr-gi commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1877748000)
I feel like this makes sense conceptually, but I have similar concerns to @stickies-v @mzumsande and @naumenkogs with it.
If we could approach this without the need to introduce an additional permission I'd be happier, since it seems a big change for a narrow use case with a potential workaround by just accepting more than 38 total connections (plus `N` for the nobans). I also wonder how this plays out with #27114, given this would be an `in` only permission.
Given this only triggers und
...
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1877748000)
I feel like this makes sense conceptually, but I have similar concerns to @stickies-v @mzumsande and @naumenkogs with it.
If we could approach this without the need to introduce an additional permission I'd be happier, since it seems a big change for a narrow use case with a potential workaround by just accepting more than 38 total connections (plus `N` for the nobans). I also wonder how this plays out with #27114, given this would be an `in` only permission.
Given this only triggers und
...
💬 LarryRuane commented on pull request "bitcoin-cli help detail to show full help for all RPCs":
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1877751909)
> Concept NACK on the top commit: having each one in a separate file is nice.
I can remove it, but may I ask why, I assume that the purpose of the test writing to the files is in case something goes wrong in the formatting of the strings (and the test fails), the files might help the developer identify the problem. (Otherwise why not just write to `/dev/null`, which would still test the string formatting?) When the test completes successfully, all these files are deleted anyway.
If this PR
...
(https://github.com/bitcoin/bitcoin/pull/29163#issuecomment-1877751909)
> Concept NACK on the top commit: having each one in a separate file is nice.
I can remove it, but may I ask why, I assume that the purpose of the test writing to the files is in case something goes wrong in the formatting of the strings (and the test fails), the files might help the developer identify the problem. (Otherwise why not just write to `/dev/null`, which would still test the string formatting?) When the test completes successfully, all these files are deleted anyway.
If this PR
...
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1442251493)
Good catch, fixed.
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1442251493)
Good catch, fixed.
💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1442251567)
Done
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1442251567)
Done
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1442254458)
I don't think so because it's expected to not modify `nLocalServices`. Doesn't it?
```cpp
/**
* Services this node offers.
*
* This data is replicated in each Peer instance we create.
*
* This data is not marked const, but after being set it should not
* change.
*
* \sa Peer::our_services
*/
ServiceFlags nLocalServices;
```
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1442254458)
I don't think so because it's expected to not modify `nLocalServices`. Doesn't it?
```cpp
/**
* Services this node offers.
*
* This data is replicated in each Peer instance we create.
*
* This data is not marked const, but after being set it should not
* change.
*
* \sa Peer::our_services
*/
ServiceFlags nLocalServices;
```
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1442254695)
I will address it in the next push.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1442254695)
I will address it in the next push.
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1442255302)
I don't think so because some tests rely on `-whitelist` for other purposes.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1442255302)
I don't think so because some tests rely on `-whitelist` for other purposes.