💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160655138)
nit: actual newlines are used (`0A`), not newline escape sequences (`5C 6E`), thus:
```suggestion
{RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to restoring the wallet. Multiple messages will be delimited by newlines. (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
```
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160655138)
nit: actual newlines are used (`0A`), not newline escape sequences (`5C 6E`), thus:
```suggestion
{RPCResult::Type::STR, "warning", /*optional=*/true, "Warning messages, if any, related to restoring the wallet. Multiple messages will be delimited by newlines. (DEPRECATED, returned only if config option -deprecatedrpc=walletwarningfield is passed.)"},
```
💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160650322)
nit, style: this indentation is misleading because the two opening `{`s have the same indentation, but actually the second one is nested in the first. This way it gives the wrong impression that there is a closing `}` at the end of the first line. I guess if you run `clang-format` it will totally change everything (ie the surrounding "100" lines)?
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160650322)
nit, style: this indentation is misleading because the two opening `{`s have the same indentation, but actually the second one is nested in the first. This way it gives the wrong impression that there is a closing `}` at the end of the first line. I guess if you run `clang-format` it will totally change everything (ie the surrounding "100" lines)?
💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160664795)
nit: this would needlessly fail if a new unrelated field is added. Here we actually care that `warning` is not in the list, something like `assert("warning" not in result);`
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160664795)
nit: this would needlessly fail if a new unrelated field is added. Here we actually care that `warning` is not in the list, something like `assert("warning" not in result);`
💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160671590)
Commit 363b422983e01a532df67b2432cb13e47bf092e0 `rpc: rm unneeded UniValue copies in importmulti and importdescriptors` is useless. There is not any copying happening and the old and the new code are the same wrt how many objects are being created and destroyed.
```cpp
/* The rule of five. */
class R5
{
public:
R5(int x, double y)
{
x_ = x;
y_ = y;
std::cout << "R5::R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
}
~R5()
...
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160671590)
Commit 363b422983e01a532df67b2432cb13e47bf092e0 `rpc: rm unneeded UniValue copies in importmulti and importdescriptors` is useless. There is not any copying happening and the old and the new code are the same wrt how many objects are being created and destroyed.
```cpp
/* The rule of five. */
class R5
{
public:
R5(int x, double y)
{
x_ = x;
y_ = y;
std::cout << "R5::R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
}
~R5()
...
💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160680602)
My code is very similar to the example in https://en.cppreference.com/w/cpp/language/copy_elision. From that doc it seems compilers are not required to do that optimization, which was my thinking. Anyway, if all supported compilers do that optimization, then this change is still unnecessary.
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1160680602)
My code is very similar to the example in https://en.cppreference.com/w/cpp/language/copy_elision. From that doc it seems compilers are not required to do that optimization, which was my thinking. Anyway, if all supported compilers do that optimization, then this change is still unnecessary.
💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500309723)
@Sjors That filter problem should've been fixed by https://github.com/bitcoin/bitcoin/pull/27358/commits/4b23b488d2c5662215d78e4963ef5a2b86b4e25b, my comment was from before then. Is what's now in master still broken for you?
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500309723)
@Sjors That filter problem should've been fixed by https://github.com/bitcoin/bitcoin/pull/27358/commits/4b23b488d2c5662215d78e4963ef5a2b86b4e25b, my comment was from before then. Is what's now in master still broken for you?
💬 Sjors commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500348764)
@theuni yes. On Intel macOS 13.3:
```
% ./contrib/verifybinaries/verify.py pub 24.0.1-osx
…
[ERROR] no files matched the platform specified
```
Ditto for `24.0.1-macos`
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500348764)
@theuni yes. On Intel macOS 13.3:
```
% ./contrib/verifybinaries/verify.py pub 24.0.1-osx
…
[ERROR] no files matched the platform specified
```
Ditto for `24.0.1-macos`
💬 ryanofsky commented on issue "Permission to comment on closed PRs":
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1500359061)
Could a maintainers unlock an old PR of mine #21353? I wanted to add a note on [#21353 `f5ba424c`](https://github.com/bitcoin/bitcoin/pull/21353/commits/f5ba424cd44619d9b9be88b8593d69a7ba96db26) line 3774 that there was an unintented change of behavior in that commit.
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1500359061)
Could a maintainers unlock an old PR of mine #21353? I wanted to add a note on [#21353 `f5ba424c`](https://github.com/bitcoin/bitcoin/pull/21353/commits/f5ba424cd44619d9b9be88b8593d69a7ba96db26) line 3774 that there was an unintented change of behavior in that commit.
📝 fanquake unlocked a pull request: "interfaces: Stop exposing wallet destdata to gui"
(https://github.com/bitcoin/bitcoin/pull/21353)
Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.
There are no changes in behavior.
(https://github.com/bitcoin/bitcoin/pull/21353)
Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.
There are no changes in behavior.
💬 fanquake commented on issue "Permission to comment on closed PRs":
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1500359805)
> Could a maintainers unlock an old PR of mine https://github.com/bitcoin/bitcoin/pull/21353?
Done.
(https://github.com/bitcoin/bitcoin/issues/27234#issuecomment-1500359805)
> Could a maintainers unlock an old PR of mine https://github.com/bitcoin/bitcoin/pull/21353?
Done.
💬 ryanofsky commented on pull request "interfaces: Stop exposing wallet destdata to gui":
(https://github.com/bitcoin/bitcoin/pull/21353#discussion_r1160766690)
In commit "wallet: Add IsAddressUsed / SetAddressUsed methods" (f5ba424cd44619d9b9be88b8593d69a7ba96db26)
Note: behavior accidentally changed here. Used addresses previously were written with "p" values, now they are written with "1" values. The change should not be significant because the values are currently ignored, but future code should continue to treat "1" and "p" the same
(https://github.com/bitcoin/bitcoin/pull/21353#discussion_r1160766690)
In commit "wallet: Add IsAddressUsed / SetAddressUsed methods" (f5ba424cd44619d9b9be88b8593d69a7ba96db26)
Note: behavior accidentally changed here. Used addresses previously were written with "p" values, now they are written with "1" values. The change should not be significant because the values are currently ignored, but future code should continue to treat "1" and "p" the same
🤔 ryanofsky reviewed a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217#pullrequestreview-1376251729)
~This PR has 3 acks so maybe it is ready to be merged.~ I think this PR basically looks good as is but it would be a little better if it avoided changing behavior for refund addresses in translateTransactionType (see comment below)
(https://github.com/bitcoin/bitcoin/pull/27217#pullrequestreview-1376251729)
~This PR has 3 acks so maybe it is ready to be merged.~ I think this PR basically looks good as is but it would be a little better if it avoided changing behavior for refund addresses in translateTransactionType (see comment below)
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1160747569)
In commit "wallet: Replace use of purpose strings with an enum" (ccddce7582a51fd2857ce4d142076167631e9cfe)
It would be good to make it more explicit that value is null for change addresses. Could say:
```c++
/**
* Address label which is always nullopt for change addresses. For sending
* and receiving addresses, it will be set to an arbitrary label string
* provided by the user, or to "", which is the default label. The presence
* or absence of a label is used to distinguish change
...
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1160747569)
In commit "wallet: Replace use of purpose strings with an enum" (ccddce7582a51fd2857ce4d142076167631e9cfe)
It would be good to make it more explicit that value is null for change addresses. Could say:
```c++
/**
* Address label which is always nullopt for change addresses. For sending
* and receiving addresses, it will be set to an arbitrary label string
* provided by the user, or to "", which is the default label. The presence
* or absence of a label is used to distinguish change
...
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1160759069)
In commit "wallet: Replace use of purpose strings with an enum" (ccddce7582a51fd2857ce4d142076167631e9cfe)
To be more accurate, comment should say `with values set to "1" or "p" if present`. Value was inadvertently changed from `"p"` to `"1"` in f5ba424cd44619d9b9be88b8593d69a7ba96db26 from #21353
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1160759069)
In commit "wallet: Replace use of purpose strings with an enum" (ccddce7582a51fd2857ce4d142076167631e9cfe)
To be more accurate, comment should say `with values set to "1" or "p" if present`. Value was inadvertently changed from `"p"` to `"1"` in f5ba424cd44619d9b9be88b8593d69a7ba96db26 from #21353
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1160736280)
In commit "wallet: Replace use of purpose strings with an enum" (ccddce7582a51fd2857ce4d142076167631e9cfe)
re: https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1149457810
I think I agree with the suggestion to use a switch statement here. Also:
- It looks like behavior of this code has been changed unintentionally to no longer return hidden in the "refund" case.
- The "if purpose not set, guess" logic is no longer here needed because it is implemented at the lower layer
So
...
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1160736280)
In commit "wallet: Replace use of purpose strings with an enum" (ccddce7582a51fd2857ce4d142076167631e9cfe)
re: https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1149457810
I think I agree with the suggestion to use a switch statement here. Also:
- It looks like behavior of this code has been changed unintentionally to no longer return hidden in the "refund" case.
- The "if purpose not set, guess" logic is no longer here needed because it is implemented at the lower layer
So
...
📝 fanquake opened a pull request: "test: LLVM/Clang 16 for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27436)
Similar to other CI infra changes we've made recently. Move to LLVM/Clang 16 for the MSAN jobs (which is currently using LLVM 12).
(https://github.com/bitcoin/bitcoin/pull/27436)
Similar to other CI infra changes we've made recently. Move to LLVM/Clang 16 for the MSAN jobs (which is currently using LLVM 12).
💬 theuni commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500389847)
@Sjors the filter works almost like a grep, so it depends on the release. Starting with v23 the apple binaries were renamed.
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500389847)
@Sjors the filter works almost like a grep, so it depends on the release. Starting with v23 the apple binaries were renamed.
⚠️ Bandarfaqih opened an issue: "Enhancing Privacy in the Bitcoin Protocol."
(https://github.com/bitcoin/bitcoin/issues/27437)
This BIP proposes the direct implementation of several privacy and anonymity-enhancing features in the Bitcoin protocol, including CoinJoin, zk-SNARKs, ring signatures, and stealth addresses directly into the Bitcoin protocol. These features were envisioned by Satoshi Nakamoto and other contributors as essential tools for preserving user privacy on the network.
Motivation
Bitcoin's current level of privacy and anonymity is limited due to its public ledger, which allows anyone to view all trans
...
(https://github.com/bitcoin/bitcoin/issues/27437)
This BIP proposes the direct implementation of several privacy and anonymity-enhancing features in the Bitcoin protocol, including CoinJoin, zk-SNARKs, ring signatures, and stealth addresses directly into the Bitcoin protocol. These features were envisioned by Satoshi Nakamoto and other contributors as essential tools for preserving user privacy on the network.
Motivation
Bitcoin's current level of privacy and anonymity is limited due to its public ledger, which allows anyone to view all trans
...
💬 fanquake commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1160783338)
Could also bump this to 16.0.1 now https://github.com/llvm/llvm-project/compare/llvmorg-16.0.0...llvmorg-16.0.1.
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1160783338)
Could also bump this to 16.0.1 now https://github.com/llvm/llvm-project/compare/llvmorg-16.0.0...llvmorg-16.0.1.
💬 fanquake commented on issue "Enhancing Privacy in the Bitcoin Protocol BIP Proposal":
(https://github.com/bitcoin/bitcoin/issues/27437#issuecomment-1500396999)
Thanks. However BIPs (and related discussion) should be sent to the bitcoin-dev mailing list. Opening an issue here is not the right place. See https://github.com/bitcoin/bips#readme.
(https://github.com/bitcoin/bitcoin/issues/27437#issuecomment-1500396999)
Thanks. However BIPs (and related discussion) should be sent to the bitcoin-dev mailing list. Opening an issue here is not the right place. See https://github.com/bitcoin/bips#readme.