💬 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
...
👍 rkrux approved a pull request: "test: Assumeutxo: ensure failure when importing a snapshot twice"
(https://github.com/bitcoin/bitcoin/pull/29973#pullrequestreview-2045916735)
tACK [b259b0e](https://github.com/bitcoin/bitcoin/pull/29973/commits/b259b0e8d360726b062c4b0453d1cf5a68e1933f)
Make successful, all functional tests pass.
Short and to the point PR.
(https://github.com/bitcoin/bitcoin/pull/29973#pullrequestreview-2045916735)
tACK [b259b0e](https://github.com/bitcoin/bitcoin/pull/29973/commits/b259b0e8d360726b062c4b0453d1cf5a68e1933f)
Make successful, all functional tests pass.
Short and to the point PR.
💬 rkrux commented on pull request "test: Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#discussion_r1594183450)
I also tested with putting this block before the above for loop, and the tests passes. I wanted to see what would happen if we load another snapshot right after loading one.
(https://github.com/bitcoin/bitcoin/pull/29973#discussion_r1594183450)
I also tested with putting this block before the above for loop, and the tests passes. I wanted to see what would happen if we load another snapshot right after loading one.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1594188338)
update: in master we're now processing these `PCKG_POLICY` cases, so should be ok? @glozow
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1594188338)
update: in master we're now processing these `PCKG_POLICY` cases, so should be ok? @glozow
💬 luke-jr commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2100795137)
Actually, since these don't change on their own... maybe a separate RPC altogether? (later extended to allow some changes?)
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2100795137)
Actually, since these don't change on their own... maybe a separate RPC altogether? (later extended to allow some changes?)
💬 luke-jr commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2100841942)
Would prefer this in two steps (add PCP, then remove NAT-PMP)
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2100841942)
Would prefer this in two steps (add PCP, then remove NAT-PMP)