💬 glozow commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860904444)
> You could include this one (not detected by linter):
iff is an abbreviation of "if and only if"
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860904444)
> You could include this one (not detected by linter):
iff is an abbreviation of "if and only if"
💬 glozow commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#discussion_r1430351051)
this is done in #28948
(https://github.com/bitcoin/bitcoin/pull/29107#discussion_r1430351051)
this is done in #28948
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860915662)
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 stop relaying s
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860915662)
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 stop relaying s
...
💬 brunoerg commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860915941)
> iff is an abbreviation of "if and only if"
didn't know it, thanks! :)
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860915941)
> iff is an abbreviation of "if and only if"
didn't know it, thanks! :)
💬 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
...