💬 1440000bytes commented on pull request "Deprecate and remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1499987244)
> The [BIP35](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki) mempool P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.
It is still possible to guess mempool transactions using `getdata` by maintaining mempool with no restrictions. I dont think sharing mempool affects privacy particularly in this case as its behind `NetPermissionFlags::Memp
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1499987244)
> The [BIP35](https://github.com/bitcoin/bips/blob/master/bip-0035.mediawiki) mempool P2P message can be used to share the txids in a node's mempool (motivation in BIP35). However it is no longer used (?), is bad for privacy, and we can simplify our P2P code by removing it.
It is still possible to guess mempool transactions using `getdata` by maintaining mempool with no restrictions. I dont think sharing mempool affects privacy particularly in this case as its behind `NetPermissionFlags::Memp
...
🚀 fanquake merged a pull request: "ci: Run base install at most once"
(https://github.com/bitcoin/bitcoin/pull/27429)
(https://github.com/bitcoin/bitcoin/pull/27429)
💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500021194)
We can have a followup, to address any further issues/improvements.
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1500021194)
We can have a followup, to address any further issues/improvements.
🚀 fanquake merged a pull request: "contrib: allow multi-sig binary verification v2"
(https://github.com/bitcoin/bitcoin/pull/27358)
(https://github.com/bitcoin/bitcoin/pull/27358)
💬 josibake commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160562186)
gotcha, thanks for the explanation!
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1160562186)
gotcha, thanks for the explanation!
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1500100974)
> > If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.
>
> Since we already have short-lived connections through feelers, rotating extra block relay o
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1500100974)
> > If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How much time is that exactly? If that is short then it would be sub-optimal to open a IPv4 connection and drop it shortly after in cases where Tor connectivity is working smoothly. If it is long then we have practically increased the limit of 8 which will cause more traffic.
>
> Since we already have short-lived connections through feelers, rotating extra block relay o
...
🤔 josibake reviewed a pull request: "MiniTapscript: port Miniscript to Tapscript"
(https://github.com/bitcoin/bitcoin/pull/27255#pullrequestreview-1376054310)
Left some style nits, nothing important. Might want to run `clang-tidy`, as it ended up reformatting a lot of the new code. Also got a few warnings, not quite sure about the `FromPKBytes` warning
Still trying to wrap my head around the logic; I'll follow up with a (hopefully) more helpful review
(https://github.com/bitcoin/bitcoin/pull/27255#pullrequestreview-1376054310)
Left some style nits, nothing important. Might want to run `clang-tidy`, as it ended up reformatting a lot of the new code. Also got a few warnings, not quite sure about the `FromPKBytes` warning
Still trying to wrap my head around the logic; I'll follow up with a (hopefully) more helpful review
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160599451)
```suggestion
KeyParser parser(/*out =*/&out, /*in=*/nullptr, /*ctx=*/miniscript::MiniscriptContext::P2WSH);
```
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160599451)
```suggestion
KeyParser parser(/*out =*/&out, /*in=*/nullptr, /*ctx=*/miniscript::MiniscriptContext::P2WSH);
```
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160606743)
same comment as above re: `assert`
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160606743)
same comment as above re: `assert`
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160600034)
```suggestion
KeyParser parser(/*out=*/nullptr, /*in=*/&provider, /*ctx=*/miniscript::MiniscriptContext::P2WSH);
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160600034)
```suggestion
KeyParser parser(/*out=*/nullptr, /*in=*/&provider, /*ctx=*/miniscript::MiniscriptContext::P2WSH);
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160606636)
iirc, `assert` crashes the node. wouldn't it be better to use the `Assume` macro?
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160606636)
iirc, `assert` crashes the node. wouldn't it be better to use the `Assume` macro?
💬 josibake commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160606878)
same comment as above re: `assert`
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1160606878)
same comment as above re: `assert`
💬 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)?