💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594060918)
Let's remove this since the messages now bring the test vectors themselves, this is duplicating purpose.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594060918)
Let's remove this since the messages now bring the test vectors themselves, this is duplicating purpose.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594063834)
```suggestion
BOOST_CHECK(DecodeBase58("good"s, result, 100));
```
I understand the rational of aligning this with surrounding context, but does seem against guidelines and will look like a typo for others.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594063834)
```suggestion
BOOST_CHECK(DecodeBase58("good"s, result, 100));
```
I understand the rational of aligning this with surrounding context, but does seem against guidelines and will look like a typo for others.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594190131)
```suggestion
strTest << ": got \"" << EncodeBase58(sourcedata) << "\""
```
What about a little less verbose and taking advantage of the `strTest` string that has both input and expected outcome?
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594190131)
```suggestion
strTest << ": got \"" << EncodeBase58(sourcedata) << "\""
```
What about a little less verbose and taking advantage of the `strTest` string that has both input and expected outcome?
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594218804)
Don't need this since the test messages now bring the input vector.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594218804)
Don't need this since the test messages now bring the input vector.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594206244)
```suggestion
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
```
Testing for the same thing as before, but removing the test vector from the message don't seem to be an improvement. Potentially makes debugging more difficult.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594206244)
```suggestion
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
```
Testing for the same thing as before, but removing the test vector from the message don't seem to be an improvement. Potentially makes debugging more difficult.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594203045)
```suggestion
strTest << ": got \"" << EncodeBase58(sourcedata) << "\""
```
Same suggestion as above.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594203045)
```suggestion
strTest << ": got \"" << EncodeBase58(sourcedata) << "\""
```
Same suggestion as above.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594213673)
I was in doubt this would check if the vectors contents are equal, but it does indeed. Nice catch.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594213673)
I was in doubt this would check if the vectors contents are equal, but it does indeed. Nice catch.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594240546)
Same thing about unreported random parameter in the test. This is worse yet since there's a weird logic to get the param. It's good that you submitted this PR, the original test deserved a better look already.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594240546)
Same thing about unreported random parameter in the test. This is worse yet since there's a weird logic to get the param. It's good that you submitted this PR, the original test deserved a better look already.
💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594251265)
> Fixed, thanks! Added you as a Co-author, please check that the email is correct.
Not needed, but highly appreciated. Thanks.
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1594251265)
> Fixed, thanks! Added you as a Co-author, please check that the email is correct.
Not needed, but highly appreciated. Thanks.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594255018)
Sjors suggested here https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399137900 that I add a way to do it by transaction hash, which would be easy. Let me know if you have other suggestions...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594255018)
Sjors suggested here https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399137900 that I add a way to do it by transaction hash, which would be easy. Let me know if you have other suggestions...
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594262607)
See line 528 below: https://github.com/sdaftuar/bitcoin/blob/c49e0444e5c474d5e1ac0af89e9bc27958f3ed31/src/kernel/txgraph.cpp#L528
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594262607)
See line 528 below: https://github.com/sdaftuar/bitcoin/blob/c49e0444e5c474d5e1ac0af89e9bc27958f3ed31/src/kernel/txgraph.cpp#L528
💬 jadijadi commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-2100893822)
Please do not hesitate to ask me if any other update is needed on this from my side. Thanks.
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-2100893822)
Please do not hesitate to ask me if any other update is needed on this from my side. Thanks.
💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594266520)
I haven't been able to reproduce the issue in my local setup, but I think we may be able to improve on this (plus reduce the waiting time) by mocking the node's time after restarting it
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594266520)
I haven't been able to reproduce the issue in my local setup, but I think we may be able to improve on this (plus reduce the waiting time) by mocking the node's time after restarting it
💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594268166)
Tried that (plus some minimal improvements) in [138a2e2](https://github.com/bitcoin/bitcoin/pull/29605/commits/138a2e2a79846863fdce26102f800117f407ef97)
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594268166)
Tried that (plus some minimal improvements) in [138a2e2](https://github.com/bitcoin/bitcoin/pull/29605/commits/138a2e2a79846863fdce26102f800117f407ef97)
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594227340)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)
Might be good to add a comment noting that the "jsonrpc" version field was added in the JSON-RPC 2.0 spec, so value "1.0" is nonstandard. But it is accepted for backwards compatibility, because some old documentation (incorrectly) suggested adding it.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594227340)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)
Might be good to add a comment noting that the "jsonrpc" version field was added in the JSON-RPC 2.0 spec, so value "1.0" is nonstandard. But it is accepted for backwards compatibility, because some old documentation (incorrectly) suggested adding it.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594229793)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)
Variable name `valJsonRPC` doesn't really follow the suggested [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) which calls for snake_case. Could rename it to "jsonrpc_version"
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594229793)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)
Variable name `valJsonRPC` doesn't really follow the suggested [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) which calls for snake_case. Could rename it to "jsonrpc_version"
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594231744)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)
Technically JSON-RPC version would be more accurate the JSON version. could s/JSON/JSONRPC/ throughout this enum
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594231744)
In commit "rpc: identify JSON-RPC 2.0 requests" (6d0be75f3040c3190759c3ba6b5740897f2d3d29)
Technically JSON-RPC version would be more accurate the JSON version. could s/JSON/JSONRPC/ throughout this enum
👍 ryanofsky approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2045965827)
Code review ACK 8583c2f93d4eb020bf1e3b74860b691ec12e5eee. Looks very good! I left several comments, but none are critical and they could be followed up later.
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2045965827)
Code review ACK 8583c2f93d4eb020bf1e3b74860b691ec12e5eee. Looks very good! I left several comments, but none are critical and they could be followed up later.
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594260831)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)
This is probably supposed to say 1.1 not 1.2
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594260831)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)
This is probably supposed to say 1.1 not 1.2
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594242203)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)
I think the commit message has a typo. It says "Non-batch requests already did not return HTTP errors previously." but I think it is supposed to say "Batch requests already did not return HTTP errors previously."
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594242203)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)
I think the commit message has a typo. It says "Non-batch requests already did not return HTTP errors previously." but I think it is supposed to say "Batch requests already did not return HTTP errors previously."