💬 rkrux commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594014627)
`-32603`
Although not necessary for this PR, I am wondering how big of a lift would it be to tie these errors to the ones defined here: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/protocol.h
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594014627)
`-32603`
Although not necessary for this PR, I am wondering how big of a lift would it be to tie these errors to the ones defined here: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/protocol.h
💬 rkrux commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594008624)
Got to know from the CPP code that `-reindex-chainstate` will load up from the blk.dat files. That's why I understand only 2 more blocks are needed to exceed the work of the snapshot chain.
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1594008624)
Got to know from the CPP code that `-reindex-chainstate` will load up from the blk.dat files. That's why I understand only 2 more blocks are needed to exceed the work of the snapshot chain.
📝 hebasto opened a pull request: "build, test: Remove unused `TIMEOUT` environment variable"
(https://github.com/bitcoin/bitcoin/pull/30063)
Setting the `TIMEOUT` environment variable has been a noop in both cases since its introduction.
It seems to have been inadvertently copy-pasted from existing code. For example, in commit d80e3cbece857b293a4903ef49c4d543bb2cfb7f, it was needlessly copied from a valid case a few line above for the `qa/pull-tester/run-bitcoind-for-test.sh` script.
(https://github.com/bitcoin/bitcoin/pull/30063)
Setting the `TIMEOUT` environment variable has been a noop in both cases since its introduction.
It seems to have been inadvertently copy-pasted from existing code. For example, in commit d80e3cbece857b293a4903ef49c4d543bb2cfb7f, it was needlessly copied from a valid case a few line above for the `qa/pull-tester/run-bitcoind-for-test.sh` script.
💬 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.