💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160609483)
not a big deal, but I think it's more correct to say "CHECKMULTISIG is disabled in Tapscript" in the commit message
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160609483)
not a big deal, but I think it's more correct to say "CHECKMULTISIG is disabled in Tapscript" in the commit message
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160608237)
probably out of scope for this PR, but it would be nice to have the parser tell you why it wasn't able to parse the script, rather than just return empty
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160608237)
probably out of scope for this PR, but it would be nice to have the parser tell you why it wasn't able to parse the script, rather than just return empty
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160615879)
"warning: suggest parentheses around arithmetic in operand of ‘|’ [-Wparentheses]"
```suggestion
next_sats_diff.push_back((sats_diff[j] + subs[i]->ss.dsat.diff) | ((sats_diff[j - 1] + subs[i]->ss.sat.diff) + add_diff));
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160615879)
"warning: suggest parentheses around arithmetic in operand of ‘|’ [-Wparentheses]"
```suggestion
next_sats_diff.push_back((sats_diff[j] + subs[i]->ss.dsat.diff) | ((sats_diff[j - 1] + subs[i]->ss.sat.diff) + add_diff));
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160612094)
```suggestion
if (!parse_multi_exp(in, /*is_multi_a=*/true)) return {};
```
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160612094)
```suggestion
if (!parse_multi_exp(in, /*is_multi_a=*/true)) return {};
```
💬 Sjors commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500209136)
Followup suggestions:
1. allow user to specify the domain, e.g. `--domain=bitcoincore.org` so you don't get warnings about bitcoin.org, which only has up to version 22.0 at the moment. Conversely, it will make a future domain evacuation safer.
2. "osx" in the documentation example should be "macos" (but it doesn't work either way as @theuni found)
3. "The os/arch filters like verify.py 22.0-osx no longer work for me and I'm not quite sure why." - would be nice to restore this, or just f
...
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500209136)
Followup suggestions:
1. allow user to specify the domain, e.g. `--domain=bitcoincore.org` so you don't get warnings about bitcoin.org, which only has up to version 22.0 at the moment. Conversely, it will make a future domain evacuation safer.
2. "osx" in the documentation example should be "macos" (but it doesn't work either way as @theuni found)
3. "The os/arch filters like verify.py 22.0-osx no longer work for me and I'm not quite sure why." - would be nice to restore this, or just f
...
👍 vasild approved a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1376126094)
ACK e67da5fedd526c38e030d6d18f3678313a683dc9
Some non-blockers below.
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1376126094)
ACK e67da5fedd526c38e030d6d18f3678313a683dc9
Some non-blockers below.
💬 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