💬 natahala3 commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668988870)
>̶ ̵C̶Y̷P̷H̴E̷R̸P̴U̵N̷K̵S̵ ̴W̵R̵I̸T̷E̷ ̶C̴O̸D̴E̶
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668988870)
>̶ ̵C̶Y̷P̷H̴E̷R̸P̴U̵N̷K̵S̵ ̴W̵R̵I̸T̷E̷ ̶C̴O̸D̴E̶
💬 kravets commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1668995563)
Would it not be better to postpone removing the Core tx signatures to Deniabilization 1.1 or 2.0 version and get this safe, isolated UI centric feature merged in sooner rather than later ?
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1668995563)
Would it not be better to postpone removing the Core tx signatures to Deniabilization 1.1 or 2.0 version and get this safe, isolated UI centric feature merged in sooner rather than later ?
💬 natahala3 commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668998846)
Cypherpunks write code.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668998846)
Cypherpunks write code.
👍 MarcoFalke approved a pull request: "bench: add readblock benchmark"
(https://github.com/bitcoin/bitcoin/pull/26684#pullrequestreview-1566578747)
lgtm ACK. I think one issue could be that the bench (like for all tests) datadir is on a tmpfs and not the normal storage that blocks are stored on, so the bench is skewed toward highlighting code performance more than it matters, because block storage is generally slower (than tmpfs in memory) and the limiting factor.
Maybe a hint or comment on how to pick the datadir for this test can be added?
(https://github.com/bitcoin/bitcoin/pull/26684#pullrequestreview-1566578747)
lgtm ACK. I think one issue could be that the bench (like for all tests) datadir is on a tmpfs and not the normal storage that blocks are stored on, so the bench is skewed toward highlighting code performance more than it matters, because block storage is generally slower (than tmpfs in memory) and the limiting factor.
Maybe a hint or comment on how to pick the datadir for this test can be added?
💬 MarcoFalke commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286663976)
style-nit: Forgot to run clang-format on new code?
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286663976)
style-nit: Forgot to run clang-format on new code?
💬 natahala3 commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669000929)
Cypherpunks write code. We know that someone has to write software to defend privacy, and since we can't get privacy unless we all do, we're going to write it. We publish our code so that our fellow Cypherpunks may practice and play with it. Our code is free for all to use, worldwide. We don't much care if you don't approve of the software we write. We know that software can't be destroyed and that a widely dispersed system can't be shut down.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669000929)
Cypherpunks write code. We know that someone has to write software to defend privacy, and since we can't get privacy unless we all do, we're going to write it. We publish our code so that our fellow Cypherpunks may practice and play with it. Our code is free for all to use, worldwide. We don't much care if you don't approve of the software we write. We know that software can't be destroyed and that a widely dispersed system can't be shut down.
📝 MarcoFalke opened a pull request: " refactor: Enforce C-str fmt strings in WalletLogPrintf() "
(https://github.com/bitcoin/bitcoin/pull/28237)
All fmt functions only accept a raw C-string as argument. Thus, enforce this for callers of `WalletLogPrintf()` as well.
(https://github.com/bitcoin/bitcoin/pull/28237)
All fmt functions only accept a raw C-string as argument. Thus, enforce this for callers of `WalletLogPrintf()` as well.
💬 dangershony commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669041322)
Concept ACK
We're building a specialized wallet and often struggle on where to keep pubkeys to recreate tapscripts, using opreturn makes it much simpler
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669041322)
Concept ACK
We're building a specialized wallet and often struggle on where to keep pubkeys to recreate tapscripts, using opreturn makes it much simpler
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286696841)
Done in https://github.com/bitcoin/bitcoin/pull/28237
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286696841)
Done in https://github.com/bitcoin/bitcoin/pull/28237
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286699001)
Maybe also add an example for common systems, Ubuntu/Debian-based ones, and Fedora/CentOS-based ones, what `DLLVM_DIR` should be for them?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286699001)
Maybe also add an example for common systems, Ubuntu/Debian-based ones, and Fedora/CentOS-based ones, what `DLLVM_DIR` should be for them?
💬 MarcoFalke commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1669064576)
(The next rebase should be trivial, only a one-line include conflict, I think)
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1669064576)
(The next rebase should be trivial, only a one-line include conflict, I think)
💬 MarcoFalke commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1286722759)
```suggestion
return error("Failed to read block %s from disk",
iter_tip->GetBlockHash().ToString());
```
nit: For new code there is no need to add `__func__`, because users can set `-logsourcelocations` if they want. Also, missing include `#include "logging.h" ` for `error()`, see CI failure?
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1286722759)
```suggestion
return error("Failed to read block %s from disk",
iter_tip->GetBlockHash().ToString());
```
nit: For new code there is no need to add `__func__`, because users can set `-logsourcelocations` if they want. Also, missing include `#include "logging.h" ` for `error()`, see CI failure?
💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1669092536)
Fair, I assumed they worked like v1 notifications. Then they don't have the footprint I thought (and now I understand why it's better than v1's, like you told me on Twitter), but I hope my message conveyed that I'm not specifically concerned about BIP47 v3 (which I see as legit), but more generally about leaving the door open to easily embed data in the UTXO set:
> You should have more leeway in 2023 to redesign BIP47 v3 on top of OP_RETURN than I'll have in 2050 to run a full node **if bare
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1669092536)
Fair, I assumed they worked like v1 notifications. Then they don't have the footprint I thought (and now I understand why it's better than v1's, like you told me on Twitter), but I hope my message conveyed that I'm not specifically concerned about BIP47 v3 (which I see as legit), but more generally about leaving the door open to easily embed data in the UTXO set:
> You should have more leeway in 2023 to redesign BIP47 v3 on top of OP_RETURN than I'll have in 2050 to run a full node **if bare
...
💬 pajasevi commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1669145950)
v3 payment codes to be redesigned on top OP_RETURN would just mean switching back to v1 scheme. You would understand that if you actually understood those specifications.
I didn't come here to debate the nuances of "banning" certain transactions on P2P layer because, frankly, I don't agree with the premise. I came here to refute the nonsense that was claimed by people who should know better. And since I don't agree with the premise, the execution and the results, I totally NACK this.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1669145950)
v3 payment codes to be redesigned on top OP_RETURN would just mean switching back to v1 scheme. You would understand that if you actually understood those specifications.
I didn't come here to debate the nuances of "banning" certain transactions on P2P layer because, frankly, I don't agree with the premise. I came here to refute the nonsense that was claimed by people who should know better. And since I don't agree with the premise, the execution and the results, I totally NACK this.
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1669179392)
Anything left to do here, for a CI-only pull request with review?
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1669179392)
Anything left to do here, for a CI-only pull request with review?
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286809420)
I thought it would be more fun for reviewers if they could ACK the first use of the `mutable const` keyword in this codebase. Also, the value will be `nullptr` before and after the call to `HandleRequest`, so it seems *almost* `const` to me, but happy to change.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286809420)
I thought it would be more fun for reviewers if they could ACK the first use of the `mutable const` keyword in this codebase. Also, the value will be `nullptr` before and after the call to `HandleRequest`, so it seems *almost* `const` to me, but happy to change.
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286809802)
Nice, used your doc string. Thanks
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286809802)
Nice, used your doc string. Thanks
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286811085)
> This implementation only supports being called on parameters with a RPCArg::Default fallback.
sorry, I was missing the early return `if (!arg.isNull()) return arg;`. Fixed now. Good catch!
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286811085)
> This implementation only supports being called on parameters with a RPCArg::Default fallback.
sorry, I was missing the early return `if (!arg.isNull()) return arg;`. Fixed now. Good catch!
💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286815104)
> `std::optional`
Not sure. This requires all args to be copied again, which can be expensive, if the arg is a hex-encoded block. (https://github.com/bitcoin/bitcoin/pull/20017/files) had the same issue, btw.
I wrote some code to implement `std::string` as well (without a copy).
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286815104)
> `std::optional`
Not sure. This requires all args to be copied again, which can be expensive, if the arg is a hex-encoded block. (https://github.com/bitcoin/bitcoin/pull/20017/files) had the same issue, btw.
I wrote some code to implement `std::string` as well (without a copy).
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141)
This is wrong. The argument of `WalletLogPrintf` is never a string literal (the parameter is).
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141)
This is wrong. The argument of `WalletLogPrintf` is never a string literal (the parameter is).