📝 fanquake opened a pull request: "doc: remove Fedora libdb4-*-devel install docs"
(https://github.com/bitcoin/bitcoin/pull/28231)
These are no-longer installable on any recent Fedora (last working version was 32).
Remove the install instructions, and consolidate this section to be the same as the
Ubuntu & Debian BDB install instructions.
(https://github.com/bitcoin/bitcoin/pull/28231)
These are no-longer installable on any recent Fedora (last working version was 32).
Remove the install instructions, and consolidate this section to be the same as the
Ubuntu & Debian BDB install instructions.
💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285799742)
Not sure about the formatting change and replacing "Fedora" with "Ubuntu". But removing the `dnf install` seems fine.
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285799742)
Not sure about the formatting change and replacing "Fedora" with "Ubuntu". But removing the `dnf install` seems fine.
💬 fanquake commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285801475)
The formatting is the same as the above (hence the typo). Will fixup.
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285801475)
The formatting is the same as the above (hence the typo). Will fixup.
💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285808893)
This is still wrong. Also, I am not sure why the formatting of a section that is about to be removed needs to change at all. Just leave it as-is?
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285808893)
This is still wrong. Also, I am not sure why the formatting of a section that is about to be removed needs to change at all. Just leave it as-is?
💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285813032)
Ok, I see `dnf` actually installs 5.3 and not 5.1. It would be better to mention such changes in the message instead of claiming the section is the "same" as Ubuntu.
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285813032)
Ok, I see `dnf` actually installs 5.3 and not 5.1. It would be better to mention such changes in the message instead of claiming the section is the "same" as Ubuntu.
💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285816292)
Nvm my previous comment, it is still wrong. You are changing 5.3 to 5.1 for no reason (not the other way round).
Would be better to just leave the section as-is, unless there is a reason to change something.
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285816292)
Nvm my previous comment, it is still wrong. You are changing 5.3 to 5.1 for no reason (not the other way round).
Would be better to just leave the section as-is, unless there is a reason to change something.
💬 fanquake commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285821657)
I didn't see an issue with consolidating it to be the same as another identical section. In any case, no more fomatting changes, and fixed the typo in the Ubuntu/Debian section.
(https://github.com/bitcoin/bitcoin/pull/28231#discussion_r1285821657)
I didn't see an issue with consolidating it to be the same as another identical section. In any case, no more fomatting changes, and fixed the typo in the Ubuntu/Debian section.
💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285806857)
nit: would be helpful to specify what it throws, and perhaps use `@throws` to make it structured?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285806857)
nit: would be helpful to specify what it throws, and perhaps use `@throws` to make it structured?
💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447)
I'm not sure this is threadsafe with our `g_work_queue`. Will we not have race conditions here when multiple requests on the same `RPCHelpMan` are in the queue at the same time?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447)
I'm not sure this is threadsafe with our `g_work_queue`. Will we not have race conditions here when multiple requests on the same `RPCHelpMan` are in the queue at the same time?
💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285831220)
nit: as we're getting the parameter value, would `Arg` be a better name? Perhaps it's too similar to `RPCArg` though (which unfortunately would have been more appropriately named `RPCParam`, I think).
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285831220)
nit: as we're getting the parameter value, would `Arg` be a better name? Perhaps it's too similar to `RPCArg` though (which unfortunately would have been more appropriately named `RPCParam`, I think).
💬 stickies-v commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285838697)
I think we also want to set this to nullptr in the many cases where we throw early in this function?
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285838697)
I think we also want to set this to nullptr in the many cases where we throw early in this function?
💬 MarcoFalke commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285862343)
`RPCHelpMan` are constructed and allocated fresh for each RPC call.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285862343)
`RPCHelpMan` are constructed and allocated fresh for each RPC call.
💬 MarcoFalke commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285865972)
Why? `throw` can only happen on internal bugs. Not sure if it makes sense to write additional code to cover unreachable code.
Also, how? This can be done in the destructor, but then the point of setting it is moot.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285865972)
Why? `throw` can only happen on internal bugs. Not sure if it makes sense to write additional code to cover unreachable code.
Also, how? This can be done in the destructor, but then the point of setting it is moot.
💬 MarcoFalke commented on pull request "rpc: Add Param() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285870318)
I am leaning toward removing the "throws". This only happens on internal bugs. For example if a dev writes `self.Param<int>(99999999)` (or switches `int` to `bool` by accident).
It is the requirement of the dev to pass the correct index and the tests will check it (if they call the RPC at least once).
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285870318)
I am leaning toward removing the "throws". This only happens on internal bugs. For example if a dev writes `self.Param<int>(99999999)` (or switches `int` to `bool` by accident).
It is the requirement of the dev to pass the correct index and the tests will check it (if they call the RPC at least once).
💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1667870906)
Updated 1be04724a3ac061859118c09b90e2e15ea8d04b0 -> d8f1222ac50f089a0af29eaf8ce0555bad8366ef ([cleaveLeveldbHeaders_5](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_5) -> [cleaveLeveldbHeaders_6](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_5..cleaveLeveldbHeaders_6))
Sorry for the re-push @stickies-v, did not include two changes in my previous push by mistake that shou
...
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1667870906)
Updated 1be04724a3ac061859118c09b90e2e15ea8d04b0 -> d8f1222ac50f089a0af29eaf8ce0555bad8366ef ([cleaveLeveldbHeaders_5](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_5) -> [cleaveLeveldbHeaders_6](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_5..cleaveLeveldbHeaders_6))
Sorry for the re-push @stickies-v, did not include two changes in my previous push by mistake that shou
...
👍 dergoegge approved a pull request: "net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1565346529)
Code review ACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1565346529)
Code review ACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b
💬 dergoegge commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1285876870)
Let's just leave it as is, was more of a nit anyway
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1285876870)
Let's just leave it as is, was more of a nit anyway
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1667875787)
re-ACK https://github.com/bitcoin/bitcoin/commit/d8f1222ac50f089a0af29eaf8ce0555bad8366ef
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1667875787)
re-ACK https://github.com/bitcoin/bitcoin/commit/d8f1222ac50f089a0af29eaf8ce0555bad8366ef
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1285881301)
Fair enough. Can easily improve later if/when necessary.
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1285881301)
Fair enough. Can easily improve later if/when necessary.
💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1667881786)
@MarcoFalke could you give this another look? You've done similar work on our time type-safety, so I'd appreciate your feedback.
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1667881786)
@MarcoFalke could you give this another look? You've done similar work on our time type-safety, so I'd appreciate your feedback.