💬 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?

💬 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).

💬 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?

💬 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.

💬 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.

💬 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).

💬 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
...

👍 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

💬 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

💬 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

💬 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.

💬 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.

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285892111)
Thx, done

📝 furszy opened a pull request: "test: locked_wallet, skip default fee estimation"
(https://github.com/bitcoin/bitcoin/pull/28232)
Coming from https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284563239.

No test case in this file is meant to exercise fee estimation. All default wallets have a
custom tx fee set [here](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/test/functional/wallet_fundrawtransaction.py#L100). The only one missing is the one created for `locked_wallet`.

👍 fanquake approved a pull request: "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings"
(https://github.com/bitcoin/bitcoin/pull/27401#pullrequestreview-1565233638)
ACK 5197660e947435e510ef3ef72be8be8dee3ffa41 - checked that this fixes the warnings under Clang.

💬 fanquake commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#discussion_r1285806943)
Generally not a huge fan of (in code) compiler and c++ version specific work arounds, but I think this is use-specific enough, with a clear (future) time for removal.

✅ dergoegge closed a pull request: "net, refactor: Privatise CNode send queue"
(https://github.com/bitcoin/bitcoin/pull/27407)

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285912365)
The only check it does is, that if the default value is used, the return type must be able to represent it. (The other checks were already done before.

However, I think it is an implementation detail of `RPCHelpMan` when to check for internal bugs. Developers should just avoid writing bugs in the first place. And if they do, they'll get a helpful error message (in the tests) and they shouldn't care where it is from.

Thus, I've removed this from the docs completely.

💬 dergoegge commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1667922866)
Can be marked up for grabs

✅ dergoegge closed a pull request: "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode"
(https://github.com/bitcoin/bitcoin/pull/27050)