💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924893374)
> The default mempool policy serves a similar purpose. Can miners circumvent these limits? Absolutely, but there are harsh consequences, and ultimately miners' interests are much better served by simply playing by the rules.
What are the harsh consequences of changing mempool policies in a permissionless network?
> "Convincing more hash power to set -permitbaremultisig=0" is precisely what I'm doing right now, by lobbying for this PR to be merged into bitcoin core. Sitting on our hands and
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924893374)
> The default mempool policy serves a similar purpose. Can miners circumvent these limits? Absolutely, but there are harsh consequences, and ultimately miners' interests are much better served by simply playing by the rules.
What are the harsh consequences of changing mempool policies in a permissionless network?
> "Convincing more hash power to set -permitbaremultisig=0" is precisely what I'm doing right now, by lobbying for this PR to be merged into bitcoin core. Sitting on our hands and
...
💬 furszy commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476878728)
in 78ba0e6748d:
I know that this shouldn't fail, but it would be good to log the error if it happens and inform the user in the error message that they need to reload the wallet manually.
(same for the others)
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476878728)
in 78ba0e6748d:
I know that this shouldn't fail, but it would be good to log the error if it happens and inform the user in the error message that they need to reload the wallet manually.
(same for the others)
🤔 furszy reviewed a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1860637525)
Code review ACK 4da76ca2
Also verified that the tests fail when bug fix commits are reverted.
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1860637525)
Code review ACK 4da76ca2
Also verified that the tests fail when bug fix commits are reverted.
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924930353)
> > The default mempool policy serves a similar purpose. Can miners circumvent these limits? Absolutely, but there are harsh consequences, and ultimately miners' interests are much better served by simply playing by the rules.
>
> What are the harsh consequences of changing mempool policies in a permissionless network?
For example, Mara pool [when they mined OFAC-compliant blocks](https://www.theblock.co/linked/106865/marathon-ofac-bitcoin-mining-pool-taproot). This was clearly a hostile m
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924930353)
> > The default mempool policy serves a similar purpose. Can miners circumvent these limits? Absolutely, but there are harsh consequences, and ultimately miners' interests are much better served by simply playing by the rules.
>
> What are the harsh consequences of changing mempool policies in a permissionless network?
For example, Mara pool [when they mined OFAC-compliant blocks](https://www.theblock.co/linked/106865/marathon-ofac-bitcoin-mining-pool-taproot). This was clearly a hostile m
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924946068)
> Personally, I don't like how `TxStateInactive` is so broad. [...]
Sure, but then I'm pretty sure we need to add a `bool abandoned;` field to the `TxStateBlockConflicted` struct so we don't lose track of whether the user has abandoned the transaction.
Alternately, we could go more in the other direction, getting rid of the inactive catch-all and dividing TxStateInactive into 4 states:
- TxStateAbandonedAndConflictsWithMempool
- TxStateAbandonedAndProbablyDoesntConflictWithMempool
- T
...
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924946068)
> Personally, I don't like how `TxStateInactive` is so broad. [...]
Sure, but then I'm pretty sure we need to add a `bool abandoned;` field to the `TxStateBlockConflicted` struct so we don't lose track of whether the user has abandoned the transaction.
Alternately, we could go more in the other direction, getting rid of the inactive catch-all and dividing TxStateInactive into 4 states:
- TxStateAbandonedAndConflictsWithMempool
- TxStateAbandonedAndProbablyDoesntConflictWithMempool
- T
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924962910)
> Personally, I don't like how `TxStateInactive` is so broad. [...]
I will also add that I think dividing the TxStateInactive state instead of just adding a new field to it makes this PR considerably harder to review because you have to manually search for every line of code that is accessing the `m_state` field directly or indirectly and verify that it is either:
1. Treating the TxStateInactive and TxStateMempoolConflicted states equivalently and not changing behavior
2. Treating the TxS
...
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924962910)
> Personally, I don't like how `TxStateInactive` is so broad. [...]
I will also add that I think dividing the TxStateInactive state instead of just adding a new field to it makes this PR considerably harder to review because you have to manually search for every line of code that is accessing the `m_state` field directly or indirectly and verify that it is either:
1. Treating the TxStateInactive and TxStateMempoolConflicted states equivalently and not changing behavior
2. Treating the TxS
...
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924990135)
> For example, Mara pool when they mined OFAC-compliant blocks. This was clearly a hostile move, and was clearly done for short-term profit. They were almost immediately forced to stop because they knew hashers would move to other pools.
> Also F2Pool, same deal.
- These are the examples for **excluding** some transactions. `permitbaremultisig=1` does not exclude but **include** bare multisig transactions.
- Ocean is a good example of following a different mempool policy for some template
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924990135)
> For example, Mara pool when they mined OFAC-compliant blocks. This was clearly a hostile move, and was clearly done for short-term profit. They were almost immediately forced to stop because they knew hashers would move to other pools.
> Also F2Pool, same deal.
- These are the examples for **excluding** some transactions. `permitbaremultisig=1` does not exclude but **include** bare multisig transactions.
- Ocean is a good example of following a different mempool policy for some template
...
💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924998475)
> Sure, but then I'm pretty sure we need to add a `bool abandoned;` field to the `TxStateMempoolConflicted` struct so we don't lose track of whether the user has abandoned the transaction.
We already lose track of that if a conflict is created and that conflict confirms. Abandoned is not a permanent state - as soon as it shows back up in the mempool, it'll no longer be abandoned. As soon as a conflict appears in a block, it'll no longer be abandoned. Likewise, if a transaction shows up in the
...
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924998475)
> Sure, but then I'm pretty sure we need to add a `bool abandoned;` field to the `TxStateMempoolConflicted` struct so we don't lose track of whether the user has abandoned the transaction.
We already lose track of that if a conflict is created and that conflict confirms. Abandoned is not a permanent state - as soon as it shows back up in the mempool, it'll no longer be abandoned. As soon as a conflict appears in a block, it'll no longer be abandoned. Likewise, if a transaction shows up in the
...
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925020805)
>These are the examples for **excluding** some transactions. `permitbaremultisig=1` does not exclude but **include** bare multisig transactions.
I don't see how this is relevant. Default mempool policy can either exclude or include certain classes of transactions. Altering these defaults is defying the user community's explicitly stated wishes.
>Ocean is a good example of following a different mempool policy for some templates which **excludes** some transactions. Although it seems irrati
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925020805)
>These are the examples for **excluding** some transactions. `permitbaremultisig=1` does not exclude but **include** bare multisig transactions.
I don't see how this is relevant. Default mempool policy can either exclude or include certain classes of transactions. Altering these defaults is defying the user community's explicitly stated wishes.
>Ocean is a good example of following a different mempool policy for some templates which **excludes** some transactions. Although it seems irrati
...
💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1925021602)
> Abandoned is not a permanent state - as soon as it shows back up in the mempool, it'll no longer be abandoned.
It makes sense that a transaction in the mempool can't be treated as abandoned. But I don't see why the abandoned state be should be cleared just because a mempool heuristic happened to detect a conflict. That just seems like a bad UX and a confusion of two orthogonal concepts. The abandoned state is transient in the sense that it can be overriden by mempool events and block confir
...
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1925021602)
> Abandoned is not a permanent state - as soon as it shows back up in the mempool, it'll no longer be abandoned.
It makes sense that a transaction in the mempool can't be treated as abandoned. But I don't see why the abandoned state be should be cleared just because a mempool heuristic happened to detect a conflict. That just seems like a bad UX and a confusion of two orthogonal concepts. The abandoned state is transient in the sense that it can be overriden by mempool events and block confir
...
🚀 ryanofsky merged a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868)
(https://github.com/bitcoin/bitcoin/pull/28868)
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925168788)
@chrisguida
> For example, Mara pool [when they mined OFAC-compliant blocks](https://www.theblock.co/linked/106865/marathon-ofac-bitcoin-mining-pool-taproot). This was clearly a hostile move, and was clearly done for **short-term profit**. They were almost immediately forced to stop because they knew hashers would move to other pools.
(emphasis mine)
Why do you think Mara was mining OFAC compliant blocks for *short-term* profit? They were leaving fees on the table, and I'm unaware of a
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925168788)
@chrisguida
> For example, Mara pool [when they mined OFAC-compliant blocks](https://www.theblock.co/linked/106865/marathon-ofac-bitcoin-mining-pool-taproot). This was clearly a hostile move, and was clearly done for **short-term profit**. They were almost immediately forced to stop because they knew hashers would move to other pools.
(emphasis mine)
Why do you think Mara was mining OFAC compliant blocks for *short-term* profit? They were leaving fees on the table, and I'm unaware of a
...
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925193807)
> I don't see how this is relevant. Default mempool policy can either exclude or include certain classes of transactions.
It is relevant and I have edited the comment to make the difference more clear. There is another technical difference in excluding and including. You just need a small percentage of nodes and miners for some transactions to be included. While excluding requires almost everyone to follow the same policy.
> Altering these defaults is defying the user community's explicitl
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925193807)
> I don't see how this is relevant. Default mempool policy can either exclude or include certain classes of transactions.
It is relevant and I have edited the comment to make the difference more clear. There is another technical difference in excluding and including. You just need a small percentage of nodes and miners for some transactions to be included. While excluding requires almost everyone to follow the same policy.
> Altering these defaults is defying the user community's explicitl
...
💬 vostrnad commented on pull request "debugwindow: update session ID tooltip":
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1925201534)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1925201534)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1925228385)
I updated the numbers and rebased the pull-req.
We should get this merged for the next release. It's pretty silly to have a default mempool policy that only 30% of hash power is following.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1925228385)
I updated the numbers and rebased the pull-req.
We should get this merged for the next release. It's pretty silly to have a default mempool policy that only 30% of hash power is following.
💬 hebasto commented on issue "ci: Android NDK has too old libc++":
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1925295864)
Qt docs [claim](https://doc.qt.io/qt-6/android.html) that it supports Clang 14.0.6 (NDK r25b).
It is possible to build depends with Clang 14.0.7 ([NDK r25c](https://github.com/android/ndk/wiki/Changelog-r25)) with API level >= 33.
However, the compiler errors remain the same, unfortunately.
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1925295864)
Qt docs [claim](https://doc.qt.io/qt-6/android.html) that it supports Clang 14.0.6 (NDK r25b).
It is possible to build depends with Clang 14.0.7 ([NDK r25c](https://github.com/android/ndk/wiki/Changelog-r25)) with API level >= 33.
However, the compiler errors remain the same, unfortunately.
💬 maflcko commented on issue "ci: Android NDK has too old libc++":
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1925301630)
> Clang 14.0.6 (NDK r25b).
Sure, but the issue is the libc++, not clang. A older libc++ is included in r25, see https://github.com/android/ndk/issues/1530#issuecomment-1659055600
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1925301630)
> Clang 14.0.6 (NDK r25b).
Sure, but the issue is the libc++, not clang. A older libc++ is included in r25, see https://github.com/android/ndk/issues/1530#issuecomment-1659055600
💬 hebasto commented on issue "ci: Android NDK has too old libc++":
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1925302087)
> > Clang 14.0.6 (NDK r25b).
>
> Sure, but the issue is the libc++, not clang. A older libc++ is included in r25, see [android/ndk#1530 (comment)](https://github.com/android/ndk/issues/1530#issuecomment-1659055600)
Right. I mean, even the latest Qt codebase cannot be compiled with NDK > 25.
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1925302087)
> > Clang 14.0.6 (NDK r25b).
>
> Sure, but the issue is the libc++, not clang. A older libc++ is included in r25, see [android/ndk#1530 (comment)](https://github.com/android/ndk/issues/1530#issuecomment-1659055600)
Right. I mean, even the latest Qt codebase cannot be compiled with NDK > 25.
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925319950)
@petertodd
> Why do you think Mara was mining OFAC compliant blocks for _short-term_ profit? They were leaving fees on the table, and I'm unaware of any short term financial incentive they had to to block those transactions.
I would think this is obvious. Filtering OFAC-sanctioned transactions costs a mining pool at most [a few hundred dollars per day](https://b10c.me/observations/08-missing-sanctioned-transactions/), while [maintaining a good ESG score](https://www.mara.com/about-us#esg) a
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1925319950)
@petertodd
> Why do you think Mara was mining OFAC compliant blocks for _short-term_ profit? They were leaving fees on the table, and I'm unaware of any short term financial incentive they had to to block those transactions.
I would think this is obvious. Filtering OFAC-sanctioned transactions costs a mining pool at most [a few hundred dollars per day](https://b10c.me/observations/08-missing-sanctioned-transactions/), while [maintaining a good ESG score](https://www.mara.com/about-us#esg) a
...
📝 furszy opened a pull request: "wallet: remove unused 'accept_no_keys' arg from decryption process"
(https://github.com/bitcoin/bitcoin/pull/29375)
Found it while reviewing other PR. Couldn't contain myself from cleaning it up.
The wallet decryption process (`CheckDecryptionKey()` and `Unlock()`)
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented `SplitWalletPath`
function.
(https://github.com/bitcoin/bitcoin/pull/29375)
Found it while reviewing other PR. Couldn't contain myself from cleaning it up.
The wallet decryption process (`CheckDecryptionKey()` and `Unlock()`)
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented `SplitWalletPath`
function.