💬 MarcoFalke commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260087559)
```suggestion
UNINTERESTING,
```
Style nit: Missing comma to avoid having to touch this line again if a new value is added (unlikely).
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1260087559)
```suggestion
UNINTERESTING,
```
Style nit: Missing comma to avoid having to touch this line again if a new value is added (unlikely).
💬 murchandamus commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1631267324)
> 1. But yes, the larger the spend the more would be revealed, so it's not ideal.
Not only that, but if you spend a UTXO that’s the change from the previous transaction with the output of the next, that’s one thing. But if you spend an output with the change from fifteen transactions prior, you may heavily imply that all payments in between were also self-sends.
> * Another option would be to provide a user parameter for "likely spendable size", which we use in the chunking decision and ma
...
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1631267324)
> 1. But yes, the larger the spend the more would be revealed, so it's not ideal.
Not only that, but if you spend a UTXO that’s the change from the previous transaction with the output of the next, that’s one thing. But if you spend an output with the change from fifteen transactions prior, you may heavily imply that all payments in between were also self-sends.
> * Another option would be to provide a user parameter for "likely spendable size", which we use in the chunking decision and ma
...
👍 brunoerg approved a pull request: "fuzz: Generate process_message targets individually"
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1524893172)
ACK bbbb996c15aef7786c3a215ed16e1913b12b0f8c
lgtm! running `./test/fuzz/test_runner.py corpus process_message -g` on this PR,`target` becomes:
```sh
[('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'version'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'verack'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addr'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'sendaddrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'inv'
...
(https://github.com/bitcoin/bitcoin/pull/28066#pullrequestreview-1524893172)
ACK bbbb996c15aef7786c3a215ed16e1913b12b0f8c
lgtm! running `./test/fuzz/test_runner.py corpus process_message -g` on this PR,`target` becomes:
```sh
[('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'version'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'verack'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addr'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'addrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'sendaddrv2'}), ('process_message', {'LIMIT_TO_MESSAGE_TYPE': 'inv'
...
💬 MarcoFalke commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631312519)
> See https://github.com/theuni/bitcoin-tidy-experiments/pull/1 for an example of the kind of synchronization problems we'll have otherwise.
Wouldn't it be easier to (have an option to) self-sanity-check the match? For example, assert that more than one result was found?
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631312519)
> See https://github.com/theuni/bitcoin-tidy-experiments/pull/1 for an example of the kind of synchronization problems we'll have otherwise.
Wouldn't it be easier to (have an option to) self-sanity-check the match? For example, assert that more than one result was found?
💬 MarcoFalke commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1631316656)
PR description needs update?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1631316656)
PR description needs update?
💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260132300)
I don't actually want to make any new guarantees here. I'm just trying to make a simplification to the `ActivateBestChain` function and document the resulting behavior in release notes. Maybe the following release note would be better?
- The `-stopatheight` option now stops earlier and no longer tries to validate and connect all the downloaded blocks above the specified height on the most-work chain. This means the resulting chain is more likely to be exactly the specified height instead of l
...
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1260132300)
I don't actually want to make any new guarantees here. I'm just trying to make a simplification to the `ActivateBestChain` function and document the resulting behavior in release notes. Maybe the following release note would be better?
- The `-stopatheight` option now stops earlier and no longer tries to validate and connect all the downloaded blocks above the specified height on the most-work chain. This means the resulting chain is more likely to be exactly the specified height instead of l
...
💬 murchandamus commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260141297)
How about something along these lines?
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The transaction’s outputs specified as key-value pairs.\n"
"Payment outputs are specified in the form of \"address\":\"amount [BTC]\", e.g. \"bc1p…xyz\":\"0.001\".\n"
"A single data output may be specified in the form of \"data\":\"hex\", e.g. \"data\":\"ff00ff00ff\".\n"
"Each key may only appear once. This means that each ad
...
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260141297)
How about something along these lines?
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The transaction’s outputs specified as key-value pairs.\n"
"Payment outputs are specified in the form of \"address\":\"amount [BTC]\", e.g. \"bc1p…xyz\":\"0.001\".\n"
"A single data output may be specified in the form of \"data\":\"hex\", e.g. \"data\":\"ff00ff00ff\".\n"
"Each key may only appear once. This means that each ad
...
💬 murchandamus commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260158337)
How about:
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
```
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1260158337)
How about:
```suggestion
{"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
```
💬 achow101 commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1631361128)
ACK 0bf87476f55dceb106563156c7c8d6bfb8162e29
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1631361128)
ACK 0bf87476f55dceb106563156c7c8d6bfb8162e29
💬 murchandamus commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1631361402)
> @Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.
I think you have correctly identified that this text could be improved and we should move forward with the PR.
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1631361402)
> @Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.
I think you have correctly identified that this text could be improved and we should move forward with the PR.
🤔 mzumsande reviewed a pull request: "Bugfix: net_processing: Restore "Already requested" error for FetchBlock"
(https://github.com/bitcoin/bitcoin/pull/28055#pullrequestreview-1525047228)
> The result of this PR would be that it would return an error IFF that particular block was requested by that specific peer last. If you request the block from another peer, the behavior would be unchanged. Is that acceptable?
I think that I would find that acceptable.
I guess one alternative would be to drop the "Already requested from this peer" error that is currently not triggerable, and just forget about the prior request unconditionally on a new request.
(https://github.com/bitcoin/bitcoin/pull/28055#pullrequestreview-1525047228)
> The result of this PR would be that it would return an error IFF that particular block was requested by that specific peer last. If you request the block from another peer, the behavior would be unchanged. Is that acceptable?
I think that I would find that acceptable.
I guess one alternative would be to drop the "Already requested from this peer" error that is currently not triggerable, and just forget about the prior request unconditionally on a new request.
💬 achow101 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1260253320)
In 94d02376c853aa4835de413d3f75b8b06bcb5193 "rpc: JSON-RPC 2.0 should not respond to "notifications""
This results in 500 and a response body of "Unhandled request". I think it should actually be a 200 with no response body.
```suggestion
if (jreq.IsNotification()) {
req->WriteReply(HTTP_OK);
return true;
}
```
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1260253320)
In 94d02376c853aa4835de413d3f75b8b06bcb5193 "rpc: JSON-RPC 2.0 should not respond to "notifications""
This results in 500 and a response body of "Unhandled request". I think it should actually be a 200 with no response body.
```suggestion
if (jreq.IsNotification()) {
req->WriteReply(HTTP_OK);
return true;
}
```
🤔 jamesob reviewed a pull request: "util: Add XorFile"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1525080853)
Concept ACK
In general looks good, but I think you could really cut down on the amount of code here if you just made `XorFile` a subclass of `CAutoFile`, and made whatever small adjustments and modernizations necessary to `CAutoFile` to get that to work.
E.g. `XorFile::ignore()` seems like essentially the same code as `CAutoFile::ignore()`, just with some `std::` prefixes. Seems like code we don't want to maintain two copies of.
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1525080853)
Concept ACK
In general looks good, but I think you could really cut down on the amount of code here if you just made `XorFile` a subclass of `CAutoFile`, and made whatever small adjustments and modernizations necessary to `CAutoFile` to get that to work.
E.g. `XorFile::ignore()` seems like essentially the same code as `CAutoFile::ignore()`, just with some `std::` prefixes. Seems like code we don't want to maintain two copies of.
💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260227191)
fa2276355889ff7bcad95aa978863107c36a89f7
Minor: new function is called `detail_fread` but commit and commit message mention `detail_read`.
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260227191)
fa2276355889ff7bcad95aa978863107c36a89f7
Minor: new function is called `detail_fread` but commit and commit message mention `detail_read`.
💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260260590)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d
Might be interesting to see how statically allocating a buffer (say, as a member of `XorFile`) and reusing it might affect performance; may avoid some reallocations. Not sure how frequently this is actually called though. I expect this probably won't make a difference though, because I guess `XorFile::write()` will be called on the order of once per block.
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260260590)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d
Might be interesting to see how statically allocating a buffer (say, as a member of `XorFile`) and reusing it might affect performance; may avoid some reallocations. Not sure how frequently this is actually called though. I expect this probably won't make a difference though, because I guess `XorFile::write()` will be called on the order of once per block.
💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260249626)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d
Should these serialization methods have similar `if (!m_file)` guards as `AutoFile` does?
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260249626)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d
Should these serialization methods have similar `if (!m_file)` guards as `AutoFile` does?
💬 jamesob commented on pull request "util: Add XorFile":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260252291)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d
Why not throw a `std::ios_base::failure()` here? From what I can tell, we'd never expect to get back a negative error value.
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1260252291)
fa9325a14584be1c5aa6b4cdefbdaa69b27e402d
Why not throw a `std::ios_base::failure()` here? From what I can tell, we'd never expect to get back a negative error value.
💬 achow101 commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1631522931)
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1631522931)
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
🚀 achow101 merged a pull request: "test: refactor: deduplicate legacy ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28025)
(https://github.com/bitcoin/bitcoin/pull/28025)
💬 theuni commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631605962)
Sure. Now that I think about it, we could just keep|copy the test suite in Core rather than|in addition to the plugin repo.
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1631605962)
Sure. Now that I think about it, we could just keep|copy the test suite in Core rather than|in addition to the plugin repo.