💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1593964758)
Here the caught `UniValue&` is not explicitly `std::move`d. But on line 272 it is. Is that intentional?
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1593964758)
Here the caught `UniValue&` is not explicitly `std::move`d. But on line 272 it is. Is that intentional?
🤔 cbergqvist reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2045544882)
reACK 8583c2f93d4eb020bf1e3b74860b691ec12e5eee
Ran most functional tests including `interface_rpc.py` without any failures. Ran `make check` without failures or skips.
Happy that the `version` parameters to `JSONRPCReplyObj()` etc are now explicit.
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2045544882)
reACK 8583c2f93d4eb020bf1e3b74860b691ec12e5eee
Ran most functional tests including `interface_rpc.py` without any failures. Ran `make check` without failures or skips.
Happy that the `version` parameters to `JSONRPCReplyObj()` etc are now explicit.
💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594034591)
Might be more correct & compatible to append `"\r\n"`. See https://stackoverflow.com/questions/5757290/http-header-line-break-style
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594034591)
Might be more correct & compatible to append `"\r\n"`. See https://stackoverflow.com/questions/5757290/http-header-line-break-style
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594082052)
That's a reasonable comment but I kept the single character which is how it worked on master (see below). I see a few examples of `\r\n` in the codebase but its rare and I'm not sure how it's decided.
https://github.com/bitcoin/bitcoin/blob/63d0b930f821132badd804822a46232a5f98bbef/src/rpc/request.cpp#L52-L56
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594082052)
That's a reasonable comment but I kept the single character which is how it worked on master (see below). I see a few examples of `\r\n` in the codebase but its rare and I'm not sure how it's decided.
https://github.com/bitcoin/bitcoin/blob/63d0b930f821132badd804822a46232a5f98bbef/src/rpc/request.cpp#L52-L56
💬 maflcko commented on pull request "build, test: Remove unused `TIMEOUT` environment variable":
(https://github.com/bitcoin/bitcoin/pull/30063#issuecomment-2100654472)
utACK 189d0da3f6f561c808fdd9fbd4dfd34ccfa23fe1
(https://github.com/bitcoin/bitcoin/pull/30063#issuecomment-2100654472)
utACK 189d0da3f6f561c808fdd9fbd4dfd34ccfa23fe1
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594099660)
Good catch thanks. I'll need @ryanofsky to weigh in here because I keep misunderstanding c++ move semantics. But here's what I think is happening:
In `JSONRPCReplyObj()` in `request.cpp` the really import move-instead-of-copy is when we call `reply.pushKV(...)`: https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64
That makes me think that the `std::move()` currently in `JSONErrorReply()` in `httprpc.cpp` is unnecessary: https://github.
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594099660)
Good catch thanks. I'll need @ryanofsky to weigh in here because I keep misunderstanding c++ move semantics. But here's what I think is happening:
In `JSONRPCReplyObj()` in `request.cpp` the really import move-instead-of-copy is when we call `reply.pushKV(...)`: https://github.com/pinheadmz/bitcoin/blob/8583c2f93d4eb020bf1e3b74860b691ec12e5eee/src/rpc/request.cpp#L64
That makes me think that the `std::move()` currently in `JSONErrorReply()` in `httprpc.cpp` is unnecessary: https://github.
...
💬 sipa commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-2100691492)
The discussion here led to #28824 being opened, where more discussion happened, and the idea evolved further. Having re-read things there, I have a concrete proposal that perhaps goes a bit further, so I'm posting it here to keep PR discussion about the implementation.
The goal is addressing ambiguity and readability, but not necessarily compatibility (because being compatibility with something broken is silly).
### Decoding rules
* If the asm string in its entirety is an **even length
...
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-2100691492)
The discussion here led to #28824 being opened, where more discussion happened, and the idea evolved further. Having re-read things there, I have a concrete proposal that perhaps goes a bit further, so I'm posting it here to keep PR discussion about the implementation.
The goal is addressing ambiguity and readability, but not necessarily compatibility (because being compatibility with something broken is silly).
### Decoding rules
* If the asm string in its entirety is an **even length
...
💬 sipa commented on pull request "Fix ASM ambiguity":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-2100693203)
Attempt to revive the discussion: https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-2100691492
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-2100693203)
Attempt to revive the discussion: https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-2100691492
💬 0xB10C commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2100699418)
Cool! Concept ACK.
I've added prelimarly support for this in the static HTML page on https://addrman.observer. You can now load the new `getrawaddrman` dumps and it will show you the AS and source AS. You can also mouse-over highlight by AS and source ASN.
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2100699418)
Cool! Concept ACK.
I've added prelimarly support for this in the static HTML page on https://addrman.observer. You can now load the new `getrawaddrman` dumps and it will show you the AS and source AS. You can also mouse-over highlight by AS and source ASN.
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1594092791)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593014537
> A RAII wrapper for the ECC state sounds like a nice improvement.
You probably saw this, but just for anyone else reading, this is implemented in the [branch](https://github.com/ryanofsky/bitcoin/commits/pr/ecc) posted https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1594092791)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593014537
> A RAII wrapper for the ECC state sounds like a nice improvement.
You probably saw this, but just for anyone else reading, this is implemented in the [branch](https://github.com/ryanofsky/bitcoin/commits/pr/ecc) posted https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944
💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1594093371)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593012869
> Some of the option fields do currently hold references and it might be a good idea to move ownership of those referred values to the kernel Context eventually.
Fair enough, new members could be added and kernel::Context could become something more like what was originally described, and the new comment I suggested would need to get updated in that case. To be sure, I don't think any changes need to be made as part o
...
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1594093371)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593012869
> Some of the option fields do currently hold references and it might be a good idea to move ownership of those referred values to the kernel Context eventually.
Fair enough, new members could be added and kernel::Context could become something more like what was originally described, and the new comment I suggested would need to get updated in that case. To be sure, I don't think any changes need to be made as part o
...
💬 0xB10C commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1594126015)
nit: I always found this confusing as I read `mapped as <something>` and not `mapped *A*utonomous*S*ystem` - don't have a concrete idea for a name and I guess it makes sense to use the same as in `getpeerinfo`.
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1594126015)
nit: I always found this confusing as I read `mapped as <something>` and not `mapped *A*utonomous*S*ystem` - don't have a concrete idea for a name and I guess it makes sense to use the same as in `getpeerinfo`.
💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2100723507)
I wonder if a sub-object would be a good idea before we start adding more settings here. There's at least also `m_expiry` that would make sense to add too.
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2100723507)
I wonder if a sub-object would be a good idea before we start adding more settings here. There's at least also `m_expiry` that would make sense to add too.
💬 sr-gi commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#issuecomment-2100723722)
@maflcko would you mind taking a look at this (given you reviewed the original PR for this fuzz test)?
(https://github.com/bitcoin/bitcoin/pull/29974#issuecomment-2100723722)
@maflcko would you mind taking a look at this (given you reviewed the original PR for this fuzz test)?
💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1594145863)
Well, you added "max" in front. I guess it makes sense, but might be surprising.
It's not clear what compatibility would mean before Core has fixed it to actually work.
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1594145863)
Well, you added "max" in front. I guess it makes sense, but might be surprising.
It's not clear what compatibility would mean before Core has fixed it to actually work.
💬 mzumsande commented on issue "Consideration of a Code of Conduct and/or written rules for moderation":
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2100732287)
> Also: I would like to propose someone opens an issue for follow-up discussion in the meta repo, links to that issue here, and then closes this issue. I think moving meta talk to meta makes sense.
There already exists one: https://github.com/bitcoin-core/meta/issues/1
(https://github.com/bitcoin/bitcoin/issues/29507#issuecomment-2100732287)
> Also: I would like to propose someone opens an issue for follow-up discussion in the meta repo, links to that issue here, and then closes this issue. I think moving meta talk to meta makes sense.
There already exists one: https://github.com/bitcoin-core/meta/issues/1
💬 sipa commented on issue "libxcb-xinerama0 Library required by bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2100737176)
@nimrare The short answer here is that there is no way around trusting your operating system's libraries. Even if all userspace things would be statically linked, you're still relying on your kernel for example. And it turns out that for interacting with graphics subsystems of your operating system, dynamic libraries are practically the only solution, as statically-linked ones would pretty much only work on the exact system they were compiled for.
(https://github.com/bitcoin/bitcoin/issues/30061#issuecomment-2100737176)
@nimrare The short answer here is that there is no way around trusting your operating system's libraries. Even if all userspace things would be statically linked, you're still relying on your kernel for example. And it turns out that for interacting with graphics subsystems of your operating system, dynamic libraries are practically the only solution, as statically-linked ones would pretty much only work on the exact system they were compiled for.
📝 vasild opened a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064)
It is expected to have some Bitcoin nodes unreachable some of the time. A failure to connect to an IPv4 or IPv6 node is already properly logged under category=net/severity=debug. Do the same when a connection fails when using a SOCKS5 proxy. This could be either to an .onion address or to an IPv4 or IPv6 address (via a Tor exit node).
Related: https://github.com/bitcoin/bitcoin/issues/29759
(https://github.com/bitcoin/bitcoin/pull/30064)
It is expected to have some Bitcoin nodes unreachable some of the time. A failure to connect to an IPv4 or IPv6 node is already properly logged under category=net/severity=debug. Do the same when a connection fails when using a SOCKS5 proxy. This could be either to an .onion address or to an IPv4 or IPv6 address (via a Tor exit node).
Related: https://github.com/bitcoin/bitcoin/issues/29759
💬 vasild commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2100763683)
> There is an inconsistency wrt logging connection failures...
Addressed in https://github.com/bitcoin/bitcoin/pull/30064
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2100763683)
> There is an inconsistency wrt logging connection failures...
Addressed in https://github.com/bitcoin/bitcoin/pull/30064
💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594138363)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1593964758
I think @cbergqvist is right here and `e` should be replaced by `std::move(e)` on line 244.
The `JSONRPCReplyObj` signature is:
```c++
UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONVersion json_version);
```
So without `std::move(e)`, the `error` parameter will be initialized by making a copy of `e` instead of moving from `e`. This is because `e` is an lvalue, while
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594138363)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1593964758
I think @cbergqvist is right here and `e` should be replaced by `std::move(e)` on line 244.
The `JSONRPCReplyObj` signature is:
```c++
UniValue JSONRPCReplyObj(UniValue result, UniValue error, std::optional<UniValue> id, JSONVersion json_version);
```
So without `std::move(e)`, the `error` parameter will be initialized by making a copy of `e` instead of moving from `e`. This is because `e` is an lvalue, while
...