💬 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
...
💬 ryanofsky commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934434005)
> Not sure if it so easy. On 32-bit platforms, `nTx` currently starts at byte 60, so making it 16-bit will still eat 32-bit with padding.
I didn't test on a 32-bit platform, but aren't bitfields a good solution to this? The following seems to work for me, increasing `nChainTx` range without increasing `CBlockIndex` size on x86_64 (144 bytes):
<details><summary>diff</summary>
<p>
```diff
--- a/src/chain.h
+++ b/src/chain.h
@@ -178,7 +178,7 @@ public:
//! Note: this value is f
...
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1934434005)
> Not sure if it so easy. On 32-bit platforms, `nTx` currently starts at byte 60, so making it 16-bit will still eat 32-bit with padding.
I didn't test on a 32-bit platform, but aren't bitfields a good solution to this? The following seems to work for me, increasing `nChainTx` range without increasing `CBlockIndex` size on x86_64 (144 bytes):
<details><summary>diff</summary>
<p>
```diff
--- a/src/chain.h
+++ b/src/chain.h
@@ -178,7 +178,7 @@ public:
//! Note: this value is f
...
🚀 hebasto merged a pull request: "release: Update translations for v27.0 soft translation string freeze"
(https://github.com/bitcoin/bitcoin/pull/29397)
(https://github.com/bitcoin/bitcoin/pull/29397)
💬 hebasto commented on issue "Release schedule for 27.0":
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-1934449827)
> ## 2024-02-01 🚧
>
> * Open Transifex translations for `v27.0`
>
> * Soft translation string freeze (no large or non-critical string changes until release)
>
> * Finalize and close translations for `v25.0`
Done.
See the announcement on the Transifex: https://app.transifex.com/bitcoin/communication/d:a5d3f4a9-8278-4435-8a19-dacfa304cc07/
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-1934449827)
> ## 2024-02-01 🚧
>
> * Open Transifex translations for `v27.0`
>
> * Soft translation string freeze (no large or non-critical string changes until release)
>
> * Finalize and close translations for `v25.0`
Done.
See the announcement on the Transifex: https://app.transifex.com/bitcoin/communication/d:a5d3f4a9-8278-4435-8a19-dacfa304cc07/
💬 moonsettler commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1934450852)
wish to change my feedback to Approach NACK, it might be more appropriate to express how i see this whole thing.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1934450852)
wish to change my feedback to Approach NACK, it might be more appropriate to express how i see this whole thing.
💬 maflcko commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934480710)
> 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...
This is macOS specific? It may be good if someone tested or bisected this.
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1934480710)
> 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...
This is macOS specific? It may be good if someone tested or bisected this.
🤔 instagibbs reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1870391990)
reviewed through 8d2c91ea40be1c33b6a64168203c51a19afcd7f8
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1870391990)
reviewed through 8d2c91ea40be1c33b6a64168203c51a19afcd7f8
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483077399)
Sub-packages of size 1 still need the check.
```suggestion
/** Must be called for every transaction, even if not v3. Not strictly necessary for transactions accepted through AcceptMultipleTransactions().
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483077399)
Sub-packages of size 1 still need the check.
```suggestion
/** Must be called for every transaction, even if not v3. Not strictly necessary for transactions accepted through AcceptMultipleTransactions().
```
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483187885)
nit for reading clarity:
```suggestion
ParentInfo(const Txid& txid, const Wtxid& wtxid, decltype(CTransaction::nVersion) version, bool has_mempool_descendant) :
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483187885)
nit for reading clarity:
```suggestion
ParentInfo(const Txid& txid, const Wtxid& wtxid, decltype(CTransaction::nVersion) version, bool has_mempool_descendant) :
```