💬 AdamISZ commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1860943639)
> If a data directory is being used, and data directory contains a bitcoin.conf file, I think the only reasonable expectation someone could have is that the bitcoin.conf file will be parsed and used just like the other files in the data directory.
and
> but surprising for the file to be ignored completely
That would be unarguable if we weren't talking about a manual setting of a different `bitcoin.conf` file. But we are, and I'd argue it's the opposite that's surprising [\*].
If it'
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1860943639)
> If a data directory is being used, and data directory contains a bitcoin.conf file, I think the only reasonable expectation someone could have is that the bitcoin.conf file will be parsed and used just like the other files in the data directory.
and
> but surprising for the file to be ignored completely
That would be unarguable if we weren't talking about a manual setting of a different `bitcoin.conf` file. But we are, and I'd argue it's the opposite that's surprising [\*].
If it'
...
🤔 darosior reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1787282671)
Concept ACK.
The interface looks good to me. Removing the dependency on `CScheduler` is also potentially helpful for fuzzing. I've for instance rebased on top of this PR a fuzz target for `ChainstateManager` i've been working on and being able to get rid of spawning and joining a thread on every run in favour of a global `DummyQueue` sped up the target by a `1.5` factor.
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1787282671)
Concept ACK.
The interface looks good to me. Removing the dependency on `CScheduler` is also potentially helpful for fuzzing. I've for instance rebased on top of this PR a fuzz target for `ChainstateManager` i've been working on and being able to get rid of spawning and joining a thread on every run in favour of a global `DummyQueue` sped up the target by a `1.5` factor.
🤔 ajtowns reviewed a pull request: "Add multiplication operator to CFeeRate"
(https://github.com/bitcoin/bitcoin/pull/29037#pullrequestreview-1787291625)
ACK 1757452cc55a6dacc62e4258043ee4d711fd281a ; lgtm
(https://github.com/bitcoin/bitcoin/pull/29037#pullrequestreview-1787291625)
ACK 1757452cc55a6dacc62e4258043ee4d711fd281a ; lgtm
💬 ajtowns commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1430387170)
Seems strange to have `operator+=` and `operator*` but no `operator*=` or `operator+`.
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1430387170)
Seems strange to have `operator+=` and `operator*` but no `operator*=` or `operator+`.
💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860994968)
>Only transactions affecting security of p2p network with DoS vectors should be non-standard.
Well, that's precisely what happens, many TXs using a false public key to add arbitrary data to the chain without the possibility of pruning.
They thus increase the cost of running a node, this can prevent people with the least computer resources from running a node and could, in the long term, seriously affect the decentralization of the network.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860994968)
>Only transactions affecting security of p2p network with DoS vectors should be non-standard.
Well, that's precisely what happens, many TXs using a false public key to add arbitrary data to the chain without the possibility of pruning.
They thus increase the cost of running a node, this can prevent people with the least computer resources from running a node and could, in the long term, seriously affect the decentralization of the network.
💬 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
...