💬 theDavidCoen commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861019399)
> Concept NACK
>
> > These kind of parameters should default to false.
> > Node operators should be active users and decide which feature they want to enable for their node.
>
> Disagree. All transactions should be relayed by default if they follow consensus rules. Only transactions affecting security of p2p network with DoS vectors should be non-standard. Users can always set it to `false` if they dont want to see bare multisig transactions in mempool.
>
> Changing defaults in core to
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861019399)
> Concept NACK
>
> > These kind of parameters should default to false.
> > Node operators should be active users and decide which feature they want to enable for their node.
>
> Disagree. All transactions should be relayed by default if they follow consensus rules. Only transactions affecting security of p2p network with DoS vectors should be non-standard. Users can always set it to `false` if they dont want to see bare multisig transactions in mempool.
>
> Changing defaults in core to
...
💬 achow101 commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1861029919)
The specific thing causing this error would be the lines:
```
2023-12-18T13:24:06Z [default wallet] Error reading wallet database: keymeta found with unexpected path
2023-12-18T13:24:06Z [default wallet] Error reading wallet database: keymeta found with unexpected path
```
This means that the stored metadata entries for a 2 keys has derivation paths of an unexpected length (`!= 3`). This error should be benign, although I think it's also erroneous since I'm pretty sure it is possible to
...
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1861029919)
The specific thing causing this error would be the lines:
```
2023-12-18T13:24:06Z [default wallet] Error reading wallet database: keymeta found with unexpected path
2023-12-18T13:24:06Z [default wallet] Error reading wallet database: keymeta found with unexpected path
```
This means that the stored metadata entries for a 2 keys has derivation paths of an unexpected length (`!= 3`). This error should be benign, although I think it's also erroneous since I'm pretty sure it is possible to
...
💬 RicYashiroLee commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861033095)
> No transaction on the Bitcoin network is spam, what a bunch of censorshipnists..
>
> This is not Bitcoin..
Concept ACK. Censorship is clearly the favorite argument of altcoiners in Bitcoin when whichever measure makes arbitrary data more cumbersome to be added to full nodes. My reasoning for ACK: regarding full nodes, always by default ALLOW OPTIONS, AND NEVER IMPOSE POSITIVE DEFAULTS. THAT IS DICTATORSHIP DISGUISED. This is the difference between positivist doctrine law, where fiat thri
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861033095)
> No transaction on the Bitcoin network is spam, what a bunch of censorshipnists..
>
> This is not Bitcoin..
Concept ACK. Censorship is clearly the favorite argument of altcoiners in Bitcoin when whichever measure makes arbitrary data more cumbersome to be added to full nodes. My reasoning for ACK: regarding full nodes, always by default ALLOW OPTIONS, AND NEVER IMPOSE POSITIVE DEFAULTS. THAT IS DICTATORSHIP DISGUISED. This is the difference between positivist doctrine law, where fiat thri
...
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1861066640)
That makes sense, so I'd change "the only reasonable expectation" to "a reasonable expectation" in my reply above.
I also think your "single slot" model of the -conf option is accurate, and we shouldn't try to merge a configuration file specified with -conf on the command line with a different `bitcoin.conf` file residing in the active data directory. But if there are two config files like that, and one is being ignored, it does seem like there is a high enough likelihood of misconfiguration
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1861066640)
That makes sense, so I'd change "the only reasonable expectation" to "a reasonable expectation" in my reply above.
I also think your "single slot" model of the -conf option is accurate, and we shouldn't try to merge a configuration file specified with -conf on the command line with a different `bitcoin.conf` file residing in the active data directory. But if there are two config files like that, and one is being ignored, it does seem like there is a high enough likelihood of misconfiguration
...
💬 kevkevinpal commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1861146424)
reACK [1757452](https://github.com/bitcoin/bitcoin/pull/29037/commits/1757452cc55a6dacc62e4258043ee4d711fd281a)
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1861146424)
reACK [1757452](https://github.com/bitcoin/bitcoin/pull/29037/commits/1757452cc55a6dacc62e4258043ee4d711fd281a)
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861203074)
> This is factually incorrect.
Node operators constantly filter out transactions even if they follow the consensus rules as per Bitcoin Core defaults, for example, if OP_RETURN > 83 byte, if mempoolminfee and minrelaytxfee are < 0.00001000 BTC, if mempoolsize > 300 MB (and the minimum fee rises consequently).
- OP_RETURN limits do not make sense. They might get removed in core or users can run binaries provided by @petertodd soon
- `mempoolminfee` and `minrelaytxfee` exist to avoid DoS
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861203074)
> This is factually incorrect.
Node operators constantly filter out transactions even if they follow the consensus rules as per Bitcoin Core defaults, for example, if OP_RETURN > 83 byte, if mempoolminfee and minrelaytxfee are < 0.00001000 BTC, if mempoolsize > 300 MB (and the minimum fee rises consequently).
- OP_RETURN limits do not make sense. They might get removed in core or users can run binaries provided by @petertodd soon
- `mempoolminfee` and `minrelaytxfee` exist to avoid DoS
...
💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861269916)
> * OP_RETURN limits do not make sense. They might get removed in core
However, it is very effective. Why will it make no sense?
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861269916)
> * OP_RETURN limits do not make sense. They might get removed in core
However, it is very effective. Why will it make no sense?
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1861447165)
[06cdad4 ](https://github.com/bitcoin/bitcoin/commit/06cdad46fb93f9cbe5badd6d3f53ebbd14299bae)to [00e5986](https://github.com/bitcoin/bitcoin/commit/00e5986d20b7a3497682e69605f2bc3471549c84):
- added missing `return` to `ProcessFixedSeeds()` in case where we already have addrs for all networks (thanks @stratospher for pointing me towards it)
- added a commit to remove `add_fixed_seeds` from `ProcessFixedSeeds()`, since the entire function is gated on this anyway.
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1861447165)
[06cdad4 ](https://github.com/bitcoin/bitcoin/commit/06cdad46fb93f9cbe5badd6d3f53ebbd14299bae)to [00e5986](https://github.com/bitcoin/bitcoin/commit/00e5986d20b7a3497682e69605f2bc3471549c84):
- added missing `return` to `ProcessFixedSeeds()` in case where we already have addrs for all networks (thanks @stratospher for pointing me towards it)
- added a commit to remove `add_fixed_seeds` from `ProcessFixedSeeds()`, since the entire function is gated on this anyway.
💬 ThongchaiDonWanon commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1861500365)
[](url)5M: 6.71% $2.59K 20/8
1H: -4.43% $77.7K 354/227
1D: 144% $545K 2.7K/1.7K
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1861500365)
[](url)5M: 6.71% $2.59K 20/8
1H: -4.43% $77.7K 354/227
1D: 144% $545K 2.7K/1.7K
💬 achow101 commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861506107)
It looks like the `SQLiteBatch` being used to write data to disk is being destroyed unexpectedly, which causes subsequent writes using the same object(??) to have the wrong db transaction state that triggers this error.
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861506107)
It looks like the `SQLiteBatch` being used to write data to disk is being destroyed unexpectedly, which causes subsequent writes using the same object(??) to have the wrong db transaction state that triggers this error.
💬 furszy commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861732127)
Yeah. The description is accurate achow. I had something for this in the past for #25297. We currently support a single db connection at time; when multiple `WalletBatch` are created on different threads, the first one destructed has priority over the others.
IIRC, I had a database manager class providing the db references, mapping the thread id to the db ref. So different threads can wait until other processes finish dumping data, acting accordantly, before making use of the db.
I think I n
...
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1861732127)
Yeah. The description is accurate achow. I had something for this in the past for #25297. We currently support a single db connection at time; when multiple `WalletBatch` are created on different threads, the first one destructed has priority over the others.
IIRC, I had a database manager class providing the db references, mapping the thread id to the db ref. So different threads can wait until other processes finish dumping data, acting accordantly, before making use of the db.
I think I n
...
💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430700959)
In 5536ace894e67322161a649f998265f461fbc8ad: I think we can improve the "No external signer ScriptPubKeyManager" message, it doesn't sound intuitive. First time I tested it prior reading the code, I got this error and I thought I didn't have any external signer, so I checked it with `enumerate`.
Perhaps: "There is no ScriptPubKeyManager for this address".
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430700959)
In 5536ace894e67322161a649f998265f461fbc8ad: I think we can improve the "No external signer ScriptPubKeyManager" message, it doesn't sound intuitive. First time I tested it prior reading the code, I got this error and I thought I didn't have any external signer, so I checked it with `enumerate`.
Perhaps: "There is no ScriptPubKeyManager for this address".
💬 MarnixCroes commented on pull request "Fix: Ensure 'Transaction View' remains disabled if no wallet is selected":
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861782254)
> > I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?
>
> I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:
> check display here
>
> First time the user opens or creates a wallet can tick the checkbox on "Mask Values", this is also persisted, so next time the wallet is open or selected will be on privacy mode. The fix was not changing the
...
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861782254)
> > I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?
>
> I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:
> check display here
>
> First time the user opens or creates a wallet can tick the checkbox on "Mask Values", this is also persisted, so next time the wallet is open or selected will be on privacy mode. The fix was not changing the
...
📝 achow101 opened a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112)
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transa
...
(https://github.com/bitcoin/bitcoin/pull/29112)
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transa
...
💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1430727818)
Yes, you are right, and I could confirm this with two of my nodes: If we connect via v2 peer and are disconnected because they have banned us, we would try once again with v1 (and then get disconnected again). Both for automatic connections, and (with this PR) manual ones. Although for manual ones it arguably probably matters less because we'll continuously retry with `-connect` and `-addnode` cli args anyway, even when everything is v1.
I don't really see a good way to avoid this, it's not lik
...
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1430727818)
Yes, you are right, and I could confirm this with two of my nodes: If we connect via v2 peer and are disconnected because they have banned us, we would try once again with v1 (and then get disconnected again). Both for automatic connections, and (with this PR) manual ones. Although for manual ones it arguably probably matters less because we'll continuously retry with `-connect` and `-addnode` cli args anyway, even when everything is v1.
I don't really see a good way to avoid this, it's not lik
...
⚠️ ajtowns opened an issue: "Test case for spending bare multisig?"
(https://github.com/bitcoin/bitcoin/issues/29113)
### Please describe the feature you'd like to see added.
I think the only place we currently test that spending a bare multisig utxo is okay is in `AreInputsStandard` in `script_p2sh_tests.cpp`, but this only checks that `AreInputsStandard()` passes, it doesn't check that there isn't some other rule preventing the spend from entering the mempool and eventually being mined.
Adding
```python
self.log.info('Spending a confirmed bare multisig is okay')
node = self.nodes[0]
...
(https://github.com/bitcoin/bitcoin/issues/29113)
### Please describe the feature you'd like to see added.
I think the only place we currently test that spending a bare multisig utxo is okay is in `AreInputsStandard` in `script_p2sh_tests.cpp`, but this only checks that `AreInputsStandard()` passes, it doesn't check that there isn't some other rule preventing the spend from entering the mempool and eventually being mined.
Adding
```python
self.log.info('Spending a confirmed bare multisig is okay')
node = self.nodes[0]
...
💬 pablomartin4btc commented on pull request "Fix: Ensure 'Transaction View' remains disabled if no wallet is selected":
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861954800)
> Being only able to change it requires a wallet to be open, to me would feel like a bug, no? that's more my "concern"
Fair enough, thanks for sharing your insights.
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1861954800)
> Being only able to change it requires a wallet to be open, to me would feel like a bug, no? that's more my "concern"
Fair enough, thanks for sharing your insights.
💬 Fiach-Dubh commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861960853)
ACK
There is evidence that a majority of people do not like or want this behavior. Having it on by default for all node runners, seems contrary to what most node runners would choose as policy if they were consciously given a choice.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1861960853)
ACK
There is evidence that a majority of people do not like or want this behavior. Having it on by default for all node runners, seems contrary to what most node runners would choose as policy if they were consciously given a choice.
🤔 amitiuttarwar reviewed a pull request: "test: create deterministic addrman in the functional tests"
(https://github.com/bitcoin/bitcoin/pull/29007#pullrequestreview-1787928129)
approach ACK. yay for finally having addrman determinism in the functional tests 🙌🏽
couple thoughts to consider-
#### 1. clearer subtest addrman setup in `rpc_net.py`
@0xB10C
> I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
>
agreed. right now, the 3 subtests (`test_addpeeraddress`, `test_getaddrmaninfo` & `test_getrawaddrman`) have a lot of implicit dependencies, making the overall test harder to understa
...
(https://github.com/bitcoin/bitcoin/pull/29007#pullrequestreview-1787928129)
approach ACK. yay for finally having addrman determinism in the functional tests 🙌🏽
couple thoughts to consider-
#### 1. clearer subtest addrman setup in `rpc_net.py`
@0xB10C
> I think it would be ideal if we could avoid the addpeeraddress test leaking into getaddrmaninfo and getrawaddrman tests.
>
agreed. right now, the 3 subtests (`test_addpeeraddress`, `test_getaddrmaninfo` & `test_getrawaddrman`) have a lot of implicit dependencies, making the overall test harder to understa
...
💬 amitiuttarwar commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430791494)
these init params are added here, but are only relevant until a couple subtests later in `test_getaddrmaninfo`. having unrelated init params can lead to unexpected behaviors, like in this recent [example](https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016). implicit dependencies between subtests can also make future development confusing, such as updating setup in an earlier test unexpectedly causing failures in seemingly unrelated tests.
leaving suggestions for improving t
...
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1430791494)
these init params are added here, but are only relevant until a couple subtests later in `test_getaddrmaninfo`. having unrelated init params can lead to unexpected behaviors, like in this recent [example](https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016). implicit dependencies between subtests can also make future development confusing, such as updating setup in an earlier test unexpectedly causing failures in seemingly unrelated tests.
leaving suggestions for improving t
...