💬 Tracachang commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1487653224)
> I was able to do what you want to do with a few bash commands (see below).
>
> I suppose we could add arguments `-file <path/to/file>` to both `listdescriptors` and `importdescriptors` that would make this even easier...?
Yes, that would be great.
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1487653224)
> I was able to do what you want to do with a few bash commands (see below).
>
> I suppose we could add arguments `-file <path/to/file>` to both `listdescriptors` and `importdescriptors` that would make this even easier...?
Yes, that would be great.
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1487679446)
> Try to set it to `false` since we are calling `ReadBody()` to read and process the result.
I think we need to re-think this fuzz test (`http_request.cpp`), as due to the new approach we have these issues with it:
- `replySent` is set to `true` from test and it will fail in WriteReply() as assert(!replySent && req);.
- even we set the above to `false`, later during `GetPeer()` as the connection is null (we can see in the fuzz test checking later `assert(service.ToStringAddrPort() == "[::]:
...
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1487679446)
> Try to set it to `false` since we are calling `ReadBody()` to read and process the result.
I think we need to re-think this fuzz test (`http_request.cpp`), as due to the new approach we have these issues with it:
- `replySent` is set to `true` from test and it will fail in WriteReply() as assert(!replySent && req);.
- even we set the above to `false`, later during `GetPeer()` as the connection is null (we can see in the fuzz test checking later `assert(service.ToStringAddrPort() == "[::]:
...
⚠️ Honzmonz opened an issue: ". Åååååååaß2Œ"
(https://github.com/bitcoin/bitcoin/issues/27359)
.
_Originally posted by @Honzmonz in https://github.com/bitcoin/bitcoin/issues/26568_
(https://github.com/bitcoin/bitcoin/issues/27359)
.
_Originally posted by @Honzmonz in https://github.com/bitcoin/bitcoin/issues/26568_
💬 Honzmonz commented on issue ". Åååååååaß2Œ":
(https://github.com/bitcoin/bitcoin/issues/27359#issuecomment-1487840458)
[_**Bank.Poland.of;”oryginalne_pisanie i projekt polskiego systemu bankowego? Oficjalny bank bitcoina;”Bank Polski”;”kanclerz banków”;”¥•”**_]()asset_growth of wealth”,”nie moja wina”;”follow docs”;”design_approved”;”methods_proven_new_modelinbanking”;”bank of Poland”bankofYœ)
(https://github.com/bitcoin/bitcoin/issues/27359#issuecomment-1487840458)
[_**Bank.Poland.of;”oryginalne_pisanie i projekt polskiego systemu bankowego? Oficjalny bank bitcoina;”Bank Polski”;”kanclerz banków”;”¥•”**_]()asset_growth of wealth”,”nie moja wina”;”follow docs”;”design_approved”;”methods_proven_new_modelinbanking”;”bank of Poland”bankofYœ)
✅ pinheadmz closed an issue: ". Åååååååaß2Œ"
(https://github.com/bitcoin/bitcoin/issues/27359)
(https://github.com/bitcoin/bitcoin/issues/27359)
:lock: fanquake locked an issue: ". Åååååååaß2Œ"
(https://github.com/bitcoin/bitcoin/issues/27359)
(https://github.com/bitcoin/bitcoin/issues/27359)
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1151002690)
Just a note to other reviews who also think this looks weird: `it` is a `const` reference being passed to `visited()` so you'd think `it` (or what it points to) can't be modified, so seems like `visited()` must have no effect. Yet we're ignoring its return value. The answer is that `visited()` does modify `it->m_epoch_marker` but that member is `mutable`. To me, it seems like this shouldn't be marked mutable because it does change the state of `*it` in a way that affects behavior, not just cachi
...
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1151002690)
Just a note to other reviews who also think this looks weird: `it` is a `const` reference being passed to `visited()` so you'd think `it` (or what it points to) can't be modified, so seems like `visited()` must have no effect. Yet we're ignoring its return value. The answer is that `visited()` does modify `it->m_epoch_marker` but that member is `mutable`. To me, it seems like this shouldn't be marked mutable because it does change the state of `*it` in a way that affects behavior, not just cachi
...
💬 LarryRuane commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1151062167)
This comment is for a possible follow-up PR, not suggesting any change here, just wanted to document this thought.
As an alternative to this flag, it might be more natural to use "chained methods" so you could write something like:
```
std::map<COutPoint, CAmount> node::MiniMiner(pool, outpoints)
.BuildMockTemplate(target_feerate)
.CalculateBumpFees();
```
(This technique is used [here](https://github.com/bitcoin/bitcoin/blob/fdd363ebd917e5916742587608d59023ced513e1/src/
...
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1151062167)
This comment is for a possible follow-up PR, not suggesting any change here, just wanted to document this thought.
As an alternative to this flag, it might be more natural to use "chained methods" so you could write something like:
```
std::map<COutPoint, CAmount> node::MiniMiner(pool, outpoints)
.BuildMockTemplate(target_feerate)
.CalculateBumpFees();
```
(This technique is used [here](https://github.com/bitcoin/bitcoin/blob/fdd363ebd917e5916742587608d59023ced513e1/src/
...
💬 fanquake commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1488017645)
Closing for now. In favour of #27358, which incorporated this, and other changes.
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1488017645)
Closing for now. In favour of #27358, which incorporated this, and other changes.
✅ fanquake closed a pull request: "fix: contrib: allow multi-sig binary verification"
(https://github.com/bitcoin/bitcoin/pull/23020)
(https://github.com/bitcoin/bitcoin/pull/23020)
💬 S3RK commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1488050411)
Concept ACK. I think we should definitely support the described use case.
For the approach, I think we already have some implementation ready for the second alternative you mentioned. #26728 adds support for a wallet master key (I think this is what you meant by "global keys"). And #23544 by @Sjors, built on top of it, adds a RPC for descriptor wallets similar to `sethdseed`.
I like the alternative better because it support more use cases:
1. storing wallet master key allows you to gene
...
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1488050411)
Concept ACK. I think we should definitely support the described use case.
For the approach, I think we already have some implementation ready for the second alternative you mentioned. #26728 adds support for a wallet master key (I think this is what you meant by "global keys"). And #23544 by @Sjors, built on top of it, adds a RPC for descriptor wallets similar to `sethdseed`.
I like the alternative better because it support more use cases:
1. storing wallet master key allows you to gene
...
💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1488079213)
Code review ACK c5fb9a4dd97b47db0e52c497a757dff943b255b1
I'm still slightly hesitating though. One question that still bothers me is whether we should expose `blank` flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.
This is a minor detail, and shouldn't block the PR, in the wo
...
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1488079213)
Code review ACK c5fb9a4dd97b47db0e52c497a757dff943b255b1
I'm still slightly hesitating though. One question that still bothers me is whether we should expose `blank` flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.
This is a minor detail, and shouldn't block the PR, in the wo
...
💬 josibake commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1488088372)
Nice, this looks much simpler now. `interface_usdt_mempool.py` is now failing but the fix is very simple: just mine a block after each test: https://github.com/josibake/bitcoin/commit/85ec64b1eb3037e8bd9349242b05615b41b87261
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1488088372)
Nice, this looks much simpler now. `interface_usdt_mempool.py` is now failing but the fix is very simple: just mine a block after each test: https://github.com/josibake/bitcoin/commit/85ec64b1eb3037e8bd9349242b05615b41b87261
💬 TheCharlatan commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488142360)
> Fetching and local verification seem to work as intended for me.
Can you post your commands? Nothing is currently working as advertised in the README for me ^^.
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488142360)
> Fetching and local verification seem to work as intended for me.
Can you post your commands? Nothing is currently working as advertised in the README for me ^^.
💬 willcl-ark commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488262940)
@TheCharlatan I see it working using e.g. the `pub` subcommand. You need to choose `pub` or `bin` to verify either the publicly-hosted binaries, or `bin` for a local binary. But you are correct the docs are now outdated...
```fish
₿ ./contrib/verifybinaries/verify.py pub bitcoin-core-23.0-x86_64
[INFO] got file https://bitcoincore.org/bin/bitcoin-core-23.0/SHA256SUMS.asc as SHA256SUMS.asc
[WARNING] https://bitcoin.org failed to provide file (https://bitcoin.org/bin/bitcoin-core-23.0/SHA256
...
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488262940)
@TheCharlatan I see it working using e.g. the `pub` subcommand. You need to choose `pub` or `bin` to verify either the publicly-hosted binaries, or `bin` for a local binary. But you are correct the docs are now outdated...
```fish
₿ ./contrib/verifybinaries/verify.py pub bitcoin-core-23.0-x86_64
[INFO] got file https://bitcoincore.org/bin/bitcoin-core-23.0/SHA256SUMS.asc as SHA256SUMS.asc
[WARNING] https://bitcoin.org failed to provide file (https://bitcoin.org/bin/bitcoin-core-23.0/SHA256
...
💬 TheCharlatan commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488268728)
> @TheCharlatan I see it working using e.g. the pub subcommand. You need to choose pub or bin to verify either the publicly-hosted binaries, or bin for a local binary. But you are correct the docs are now outdated...
Mmh, shouldn't the sample command you posted only have verified x86_64 binaries?
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488268728)
> @TheCharlatan I see it working using e.g. the pub subcommand. You need to choose pub or bin to verify either the publicly-hosted binaries, or bin for a local binary. But you are correct the docs are now outdated...
Mmh, shouldn't the sample command you posted only have verified x86_64 binaries?
💬 willcl-ark commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488270531)
Yes, there does seem to be quite a few other issues in testing currently. Just wanted to let you know how to get the thing running :)
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1488270531)
Yes, there does seem to be quite a few other issues in testing currently. Just wanted to let you know how to get the thing running :)
💬 TheCharlatan commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151491320)
nittiest of nits: There already is a `verify-commits`, so I think the directory should be `verify-binaries`.
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151491320)
nittiest of nits: There already is a `verify-commits`, so I think the directory should be `verify-binaries`.
💬 TheCharlatan commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151503567)
Where is this packaging step renaming `verify.py` to `verifybinaries.py` defined?
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151503567)
Where is this packaging step renaming `verify.py` to `verifybinaries.py` defined?
💬 TheCharlatan commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151519024)
These examples lack the the `pub` or `bin` args, no?
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151519024)
These examples lack the the `pub` or `bin` args, no?