💬 MarcoFalke commented on pull request "net_processing: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1668954581)
Looks like the tests time all out after 2 hours? Also, could use `p2p` instead of `net_processing` as the prefix, according to the docs?
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1668954581)
Looks like the tests time all out after 2 hours? Also, could use `p2p` instead of `net_processing` as the prefix, according to the docs?
💬 Psifour commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668958690)
Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting the powe
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668958690)
Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting the powe
...
💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668972547)
@RandyMcMillan That looks unrelated to the changes here. You'll have to use `make clean && make distclean`, or something similar
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668972547)
@RandyMcMillan That looks unrelated to the changes here. You'll have to use `make clean && make distclean`, or something similar
💬 MarcoFalke commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286645023)
(Personally, I think this is a style nit and either version is fine here that compiles and has CI pass. Going forward, we should either remove the section from the docs (my preference), or enforce it with clang-tidy or something else in CI)
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286645023)
(Personally, I think this is a style nit and either version is fine here that compiles and has CI pass. Going forward, we should either remove the section from the docs (my preference), or enforce it with clang-tidy or something else in CI)
💬 kravets commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1668987567)
In a tx graph ( tree really with large and randomized fan out, eventual merging of some UTXOs does not necessarily indicate that the later clustered entity is the same entity KYC’s on an exchange prior to the deniabilization fan out
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1668987567)
In a tx graph ( tree really with large and randomized fan out, eventual merging of some UTXOs does not necessarily indicate that the later clustered entity is the same entity KYC’s on an exchange prior to the deniabilization fan out
💬 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.