💬 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) :
```
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483187378)
nit for reading clarity:
```suggestion
bool m_has_mempool_descendant;
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483187378)
nit for reading clarity:
```suggestion
bool m_has_mempool_descendant;
```
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483215614)
already checked here https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R193
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483215614)
already checked here https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R193
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483245289)
I think this is more correct in the case of a reorg? We should accept if it double-spends all siblings.
```suggestion
std::all_of(children.cbegin(), children.cend(),
```
A case that exercises this stuff in `test_v3_reorg` would be nice too. Try submitting another child when there's more than one child already due to reorg.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483245289)
I think this is more correct in the case of a reorg? We should accept if it double-spends all siblings.
```suggestion
std::all_of(children.cbegin(), children.cend(),
```
A case that exercises this stuff in `test_v3_reorg` would be nice too. Try submitting another child when there's more than one child already due to reorg.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483195880)
May be easier to read if we immediately return std::nullopt if `!has_parent`? Could even push the check above https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R69
simplest case made clear
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483195880)
May be easier to read if we immediately return std::nullopt if `!has_parent`? Could even push the check above https://github.com/bitcoin/bitcoin/pull/28948/files#diff-a19c07329799a164e1e3bd89e7cd07f1a8e5e97b88f2b24c9bef17df04479c74R69
simplest case made clear
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483222917)
Might want to be extra clear there could be multiple children already due to a reorg.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483222917)
Might want to be extra clear there could be multiple children already due to a reorg.
💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853)
unrelated question: Is there any reason for this check? I understand that it is possible to leave the symbol unset and then set any symbols over the command line, but is anyone doing this, or is it realistic that anyone will ever do it?
If not, it could make sense to unconditionally include the config header?
Otherwise, I could see a bug silently sneak in when there is a typo in this line, such as `#if defined(_HAVE_CONFIG_H)` (or any other intentional or accidental typo)
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853)
unrelated question: Is there any reason for this check? I understand that it is possible to leave the symbol unset and then set any symbols over the command line, but is anyone doing this, or is it realistic that anyone will ever do it?
If not, it could make sense to unconditionally include the config header?
Otherwise, I could see a bug silently sneak in when there is a typo in this line, such as `#if defined(_HAVE_CONFIG_H)` (or any other intentional or accidental typo)
🤔 maflcko reviewed a pull request: "bitcoin-config.h includes cleanup"
(https://github.com/bitcoin/bitcoin/pull/29404#pullrequestreview-1870581585)
> Include them manually with the exception of some files in crypto:
unrelated question: Is there a reason they need to use the flag at all? The files are never compiled and linked when the flag isn't set, so making them appear "empty" when the flag isn't set is not needed?
(https://github.com/bitcoin/bitcoin/pull/29404#pullrequestreview-1870581585)
> Include them manually with the exception of some files in crypto:
unrelated question: Is there a reason they need to use the flag at all? The files are never compiled and linked when the flag isn't set, so making them appear "empty" when the flag isn't set is not needed?