💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1430137654)
In commit "test: enable JSON-RPC 2.0 calls in tests and verify expected behavior" (65c2f4448dc556e7a922ebac4abed7ddfc8d7745)
This test code is broken because the requests do not have unique ids. So when the responses come back, the later responses override earlier ones in the `result_by_id` array, the existing test cases without a version fields will not be checked, and any result they return will be treated as valid.
I also think copying and pasting existing test cases for version 1.1 and
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1430137654)
In commit "test: enable JSON-RPC 2.0 calls in tests and verify expected behavior" (65c2f4448dc556e7a922ebac4abed7ddfc8d7745)
This test code is broken because the requests do not have unique ids. So when the responses come back, the later responses override earlier ones in the `result_by_id` array, the existing test cases without a version fields will not be checked, and any result they return will be treated as valid.
I also think copying and pasting existing test cases for version 1.1 and
...
👍 kristapsk approved a pull request: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693#pullrequestreview-1787134005)
ACK cd4e5ddaf7f9bd50690952eb82228c3b328c9221
(https://github.com/bitcoin/bitcoin/pull/22693#pullrequestreview-1787134005)
ACK cd4e5ddaf7f9bd50690952eb82228c3b328c9221
💬 fanquake commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1430288696)
Yea, I think that's better, as we don't really want to copy-paste these big blocks of curl output into the middle of our our release notes.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1430288696)
Yea, I think that's better, as we don't really want to copy-paste these big blocks of curl output into the middle of our our release notes.
💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1860824375)
I agree with what you say, except the below:
> It is re-implementing existing pieces of code
Which?
> in a CPU-wasteful manner.
Isn't that just a few CPU instructions? (comparing the tip age to the current time). I agree it can be done less often than 45 seconds for the purpose of avoiding limited peers.
> a node completely isolated, lagging behind, with no relevant connections vs a node that is lagging behind but it is connected to the network and syncing up the chain
Yes, the
...
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1860824375)
I agree with what you say, except the below:
> It is re-implementing existing pieces of code
Which?
> in a CPU-wasteful manner.
Isn't that just a few CPU instructions? (comparing the tip age to the current time). I agree it can be done less often than 45 seconds for the purpose of avoiding limited peers.
> a node completely isolated, lagging behind, with no relevant connections vs a node that is lagging behind but it is connected to the network and syncing up the chain
Yes, the
...
🚀 glozow merged a pull request: "wallet, mempool: propagete `checkChainLimits` error message to wallet"
(https://github.com/bitcoin/bitcoin/pull/28863)
(https://github.com/bitcoin/bitcoin/pull/28863)
💬 glozow commented on pull request "Update doc/policy/README.md":
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1860843499)
Agree with not keeping 2 separate lists. Maybe just say "see node relay options" in `-help`.
(https://github.com/bitcoin/bitcoin/pull/29095#issuecomment-1860843499)
Agree with not keeping 2 separate lists. Maybe just say "see node relay options" in `-help`.
💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430336113)
Do you intend to address this TODO?
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1430336113)
Do you intend to address this TODO?
💬 brunoerg commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860889562)
You could include this one (not detected by linter):
https://github.com/bitcoin/bitcoin/blob/dd391944dc2d4e7dda9439647e603b41ba751b78/src/wallet/fees.cpp#L52
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1860889562)
You could include this one (not detected by linter):
https://github.com/bitcoin/bitcoin/blob/dd391944dc2d4e7dda9439647e603b41ba751b78/src/wallet/fees.cpp#L52
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1860891611)
Thanks @AdamISZ, this is great feedback. In your case it sounds like you have a `bitcoin.conf` file in the datadir, but you want it to be ignored, so I hope the error message was clear enough to explain why that configuration is ambiguous and how the ambiguity could be resolved by specifying `allowignoredconf=1`. I did want the error message to be self contained and tell you how to fix it without needing to refer to release notes. So if this wasn't the case, it sounds like some improvement could
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1860891611)
Thanks @AdamISZ, this is great feedback. In your case it sounds like you have a `bitcoin.conf` file in the datadir, but you want it to be ignored, so I hope the error message was clear enough to explain why that configuration is ambiguous and how the ambiguity could be resolved by specifying `allowignoredconf=1`. I did want the error message to be self contained and tell you how to fix it without needing to refer to release notes. So if this wasn't the case, it sounds like some improvement could
...
💬 glozow commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-1860896797)
> My initial attempt at this modified `getrawmempool` to
>
> * take a `start_sequence` to allow for filtering, and
>
> * adding the txdata (inputs outpoints + outputs (amount + scriptPubKeys)).
This seems like the best option imo. If we want to return the transaction hex (iiuc that should suffice if you are looking to get the tx data itself) we could add another level of verbosity to `getrawmempool` / `getmempoolentry`.
> That was pretty ugly however, given that the current d
...
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-1860896797)
> My initial attempt at this modified `getrawmempool` to
>
> * take a `start_sequence` to allow for filtering, and
>
> * adding the txdata (inputs outpoints + outputs (amount + scriptPubKeys)).
This seems like the best option imo. If we want to return the transaction hex (iiuc that should suffice if you are looking to get the tx data itself) we could add another level of verbosity to `getrawmempool` / `getmempoolentry`.
> That was pretty ugly however, given that the current d
...
💬 ns-xvrn commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860904280)
Concept ACK
a bare minimum default value change of an optional parameter makes lot of sense to increase the chances of keeping chainstate dataset clean
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860904280)
Concept ACK
a bare minimum default value change of an optional parameter makes lot of sense to increase the chances of keeping chainstate dataset clean
💬 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.