💬 hebasto commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151165179)
2e187ffafc9cb0490eedd079ce1cfffafecd9dba: trailing whitespace
```suggestion
- you can run the packaged `verifybinaries.py ... --import-keys` script to
```
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1151165179)
2e187ffafc9cb0490eedd079ce1cfffafecd9dba: trailing whitespace
```suggestion
- you can run the packaged `verifybinaries.py ... --import-keys` script to
```
💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1487611348)
Force-pushed changing the approach:
1. there is only a flag in `get_utxo` (`avoid_immature_coinbase=True`), removed that in other functions.
2. changed `mempool_package_limits` from `self.generate(self.nodes[0], COINBASE_MATURITY)` to `self.generate(self.wallet, COINBASE_MATURITY)`.
Seems like the fix in `mempool_package_limits` was simpler than I thought.
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1487611348)
Force-pushed changing the approach:
1. there is only a flag in `get_utxo` (`avoid_immature_coinbase=True`), removed that in other functions.
2. changed `mempool_package_limits` from `self.generate(self.nodes[0], COINBASE_MATURITY)` to `self.generate(self.wallet, COINBASE_MATURITY)`.
Seems like the fix in `mempool_package_limits` was simpler than I thought.
💬 apoelstra commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#discussion_r1151174283)
Lol. Updated the doccomment.
(https://github.com/bitcoin/bitcoin/pull/27351#discussion_r1151174283)
Lol. Updated the doccomment.
💬 dimitaracev commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#discussion_r1151194578)
Makes sense, didn't want to change much since its my first PR. Removed it with 8bba010
(https://github.com/bitcoin/bitcoin/pull/27357#discussion_r1151194578)
Makes sense, didn't want to change much since its my first PR. Removed it with 8bba010
💬 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?