💬 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];
...
💬 ryanofsky commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1483125283)
In commit "refactor: wallet, simplify addressbook migration" (595bbe6e81885d35179aba6137dc63d0e652cc1f)
IMO, this would be less confusing if `require_transfer` bool was replaced by an `copy_to_all` bool with the opposite meaning, because the thing `require_transfer` is used for in the loop below is to determining whether the address book entry is copied to all wallets or is moved from the `*this` wallet to one of the other two.
The name `require_transfer` is also misleading because the tra
...
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1483125283)
In commit "refactor: wallet, simplify addressbook migration" (595bbe6e81885d35179aba6137dc63d0e652cc1f)
IMO, this would be less confusing if `require_transfer` bool was replaced by an `copy_to_all` bool with the opposite meaning, because the thing `require_transfer` is used for in the loop below is to determining whether the address book entry is copied to all wallets or is moved from the `*this` wallet to one of the other two.
The name `require_transfer` is also misleading because the tra
...
💬 ryanofsky commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1483158375)
In commit "wallet: addressbook migration, batch db writes" (342c45f80e32b0320829ce380b5854844cd74bc8)
`write_address_book` would be a more descriptive name than `func_store_addr` and `watchonly_wallets` would be more descriptive than `wallet_vec`. The `_vec` suffix and and `func_` prefix do not do anything to help a reader of the code, IMO.
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1483158375)
In commit "wallet: addressbook migration, batch db writes" (342c45f80e32b0320829ce380b5854844cd74bc8)
`write_address_book` would be a more descriptive name than `func_store_addr` and `watchonly_wallets` would be more descriptive than `wallet_vec`. The `_vec` suffix and and `func_` prefix do not do anything to help a reader of the code, IMO.
⚠️ Crypt-iQ opened an issue: "p2p: lingering entries in `mapBlockSource`"
(https://github.com/bitcoin/bitcoin/issues/29410)
Opening an issue per https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1927184068
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the `BlockChecked` callback won't be called. This is because `ActivateBestChain` will return early since `pindexMostWork` is equal to `m_chain.Tip()`. Since the `BlockChecked` callback isn't called, `mapBlockSource` won't be removed from.
(https://github.com/bitcoin/bitcoin/issues/29410)
Opening an issue per https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1927184068
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the `BlockChecked` callback won't be called. This is because `ActivateBestChain` will return early since `pindexMostWork` is equal to `m_chain.Tip()`. Since the `BlockChecked` callback isn't called, `mapBlockSource` won't be removed from.
💬 dergoegge commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934431602)
Concept ACK
We only use `--with-asm=no` for the MSan builds in oss-fuzz but that only applies to secp, so we should be fine on that front.
Our docs (developer-notes.md and fuzzing.md) mention the use of `--disable-asm` to avoid address sanitizer failures. I've not used `--disable-asm` in any of my fuzzing and have not observed any ASan failures but they might depend on the specific sha256 optimizations used? Could also just be a relic of the past...
> If there is any desire, we can hook
...
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934431602)
Concept ACK
We only use `--with-asm=no` for the MSan builds in oss-fuzz but that only applies to secp, so we should be fine on that front.
Our docs (developer-notes.md and fuzzing.md) mention the use of `--disable-asm` to avoid address sanitizer failures. I've not used `--disable-asm` in any of my fuzzing and have not observed any ASan failures but they might depend on the specific sha256 optimizations used? Could also just be a relic of the past...
> If there is any desire, we can hook
...
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1934433190)
@instagibbs thanks for the response! I'd like to understand your concerns a bit more here, because it appears there is a disconnect here between the concerns of devs and concerns of users. What is the negative outcome that you are trying to avoid by nacking this PR? I'd like to understand the damage you think this PR will cause.
I believe I've already stated my case about the negative outcomes I'd like to avoid [in my comment above](https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1934433190)
@instagibbs thanks for the response! I'd like to understand your concerns a bit more here, because it appears there is a disconnect here between the concerns of devs and concerns of users. What is the negative outcome that you are trying to avoid by nacking this PR? I'd like to understand the damage you think this PR will cause.
I believe I've already stated my case about the negative outcomes I'd like to avoid [in my comment above](https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1
...