💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482981545)
Done
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482981545)
Done
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482981736)
Fixed
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1482981736)
Fixed
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1934137354)
Thanks for your review @josibake
I fixed all review comments
Forced pushed from a5d10367cf832497af2ac72f8c2c42dd25398c63 to 1fae882f98205afb05e96432ce57212226bc0909 [Compare changes](https://github.com/bitcoin/bitcoin/compare/a5d10367cf832497af2ac72f8c2c42dd25398c63..1fae882f98205afb05e96432ce57212226bc0909)
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1934137354)
Thanks for your review @josibake
I fixed all review comments
Forced pushed from a5d10367cf832497af2ac72f8c2c42dd25398c63 to 1fae882f98205afb05e96432ce57212226bc0909 [Compare changes](https://github.com/bitcoin/bitcoin/compare/a5d10367cf832497af2ac72f8c2c42dd25398c63..1fae882f98205afb05e96432ce57212226bc0909)
💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1934152480)
@jo-elimu that's the error the fix was to catch. Check the issue
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1934152480)
@jo-elimu that's the error the fix was to catch. Check the issue
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482999788)
I think that was a result of many changes during the path. In fact, we do not need `Initialize` anymore. We can concentrate the logic into `AddWhitelistPermissionFlags`.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482999788)
I think that was a result of many changes during the path. In fact, we do not need `Initialize` anymore. We can concentrate the logic into `AddWhitelistPermissionFlags`.
💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1934168380)
In any case, a pull request with a failing CI can not be merged. Someone will need to add coverage for the RPC, here, or in a different pull.
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1934168380)
In any case, a pull request with a failing CI can not be merged. Someone will need to add coverage for the RPC, here, or in a different pull.
📝 ryanofsky opened a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409)
**This is based on #28929.** The non-base commits are:
- [`486d39da5a3e` Add capnp serialization code for bitcoin types](https://github.com/bitcoin/bitcoin/pull/29409/commits/486d39da5a3e96791ad4033a863b606f9ae1ebc6)
- [`25e2ef26bede` Add capnp wrapper for Handler interface](https://github.com/bitcoin/bitcoin/pull/29409/commits/25e2ef26bede691ddf24f426b27987b7917437ba)
- [`22e05493c4f6` Add capnp wrapper for Chain interface](https://github.com/bitcoin/bitcoin/pull/29409/commits/22e05493c4f6
...
(https://github.com/bitcoin/bitcoin/pull/29409)
**This is based on #28929.** The non-base commits are:
- [`486d39da5a3e` Add capnp serialization code for bitcoin types](https://github.com/bitcoin/bitcoin/pull/29409/commits/486d39da5a3e96791ad4033a863b606f9ae1ebc6)
- [`25e2ef26bede` Add capnp wrapper for Handler interface](https://github.com/bitcoin/bitcoin/pull/29409/commits/25e2ef26bede691ddf24f426b27987b7917437ba)
- [`22e05493c4f6` Add capnp wrapper for Chain interface](https://github.com/bitcoin/bitcoin/pull/29409/commits/22e05493c4f6
...
👋 ryanofsky's pull request is ready for review: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409)
(https://github.com/bitcoin/bitcoin/pull/29409)
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483038171)
> Sure, that's why I suggest using `Assume`, and `return false` if there is no `activeTxn`. The callers of this should already be checking the return value to know whether the records were actually erased from the database.
ah, I completely skipped the `return false` last night.. sorry. All good, pushed.
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483038171)
> Sure, that's why I suggest using `Assume`, and `return false` if there is no `activeTxn`. The callers of this should already be checking the return value to know whether the records were actually erased from the database.
ah, I completely skipped the `return false` last night.. sorry. All good, pushed.
🤔 furszy reviewed a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1870335464)
Updated per feedback. Thanks achow.
[Small diff](https://github.com/bitcoin/bitcoin/compare/0fd7b0dafa980a5dbcee8658d18a83f4eff57085..784034f8a17d46de9eab7de6fdec673a3a3d79d8), only changed `assert` for `Assume`.
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1870335464)
Updated per feedback. Thanks achow.
[Small diff](https://github.com/bitcoin/bitcoin/compare/0fd7b0dafa980a5dbcee8658d18a83f4eff57085..784034f8a17d46de9eab7de6fdec673a3a3d79d8), only changed `assert` for `Assume`.
💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1483043429)
added this commit [9f57a2e](https://github.com/bitcoin/bitcoin/pull/29402/commits/9f57a2ec49bf2c298acccbf8d889e692bb6894f7) to change the wording from `disk` to `file`, disk is used in the file once still but I think it makes sense as it is "opened mempool file from disk"
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1483043429)
added this commit [9f57a2e](https://github.com/bitcoin/bitcoin/pull/29402/commits/9f57a2ec49bf2c298acccbf8d889e692bb6894f7) to change the wording from `disk` to `file`, disk is used in the file once still but I think it makes sense as it is "opened mempool file from disk"
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483043852)
Because we will need in `OpenNetworkConnection`.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483043852)
Because we will need in `OpenNetworkConnection`.
🤔 furszy reviewed a pull request: "validation: improve performance of CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/28339#pullrequestreview-1870367495)
Concept ACK.
Will start reviewing next week.
(https://github.com/bitcoin/bitcoin/pull/28339#pullrequestreview-1870367495)
Concept ACK.
Will start reviewing next week.
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483066874)
Nothing should happen. Should we throw an error in this case?
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483066874)
Nothing should happen. Should we throw an error in this case?
💬 kevkevinpal commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1483076491)
yup makes sense and I agree!
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1483076491)
yup makes sense and I agree!
🚀 achow101 merged a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836)
(https://github.com/bitcoin/bitcoin/pull/26836)
💬 instagibbs commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1934278288)
People are going to make them because they've been tricked into thinking they're unpruneable by scammers, they can always make slightly less efficient spam other ways that are just as bad. That's even assuming miners would leave this money on the table.
concept NACK
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1934278288)
People are going to make them because they've been tricked into thinking they're unpruneable by scammers, they can always make slightly less efficient spam other ways that are just as bad. That's even assuming miners would leave this money on the table.
concept NACK
💬 knst commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1934284747)
> Is it not worthwhile to prevent reallocation if we can
@Empact I added one more commit which adds `const` and changes way of initialization of SecureString - there were not any re-allocations, but not that's enforced by const.
Except `wallet/rpc/wallet.cpp` - there's also no reallocations, objects are not changed, but code readability would be worsened if I add there `const`.
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1934284747)
> Is it not worthwhile to prevent reallocation if we can
@Empact I added one more commit which adds `const` and changes way of initialization of SecureString - there were not any re-allocations, but not that's enforced by const.
Except `wallet/rpc/wallet.cpp` - there's also no reallocations, objects are not changed, but code readability would be worsened if I add there `const`.
🤔 ryanofsky reviewed a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1870379991)
Post-merge code review ACK 86960cdb7f75eaa2ae150914c54240d1d5ef96d1. Nice code cleanups, and use of batches to speed things up
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1870379991)
Post-merge code review ACK 86960cdb7f75eaa2ae150914c54240d1d5ef96d1. Nice code cleanups, and use of batches to speed things up
💬 ryanofsky commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1483070216)
In commit "refactor: SetAddressBookWithDB, minimize number of map lookups" (d0943315b1d00905fe7f4513b2f3f47b88a99e8f)
This isn't actually minimizing the number of lookups since it is doing two lookups in the insert case. Would replace:
```c++
std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];
...
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1483070216)
In commit "refactor: SetAddressBookWithDB, minimize number of map lookups" (d0943315b1d00905fe7f4513b2f3f47b88a99e8f)
This isn't actually minimizing the number of lookups since it is doing two lookups in the insert case. Would replace:
```c++
std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];
...