💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1570428398)
Rebased 2076d846cc917cbafe61937a99b7867067011341 -> b023c26eb8eda4c7f80ad2e7ebe1fb046e87d2ee ([pr26762.09](https://github.com/hebasto/bitcoin/commits/pr26762.09) -> [pr26762.10](https://github.com/hebasto/bitcoin/commits/pr26762.10)) due to the conflict with #27636.
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1570428398)
Rebased 2076d846cc917cbafe61937a99b7867067011341 -> b023c26eb8eda4c7f80ad2e7ebe1fb046e87d2ee ([pr26762.09](https://github.com/hebasto/bitcoin/commits/pr26762.09) -> [pr26762.10](https://github.com/hebasto/bitcoin/commits/pr26762.10)) due to the conflict with #27636.
💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1570432290)
Rebased
Also fixed the minor issues with the `test_utxo_snapshots.sh` demo script.
Since last push, I've completed a full test of the mainnet snapshot; after a few days having finished the background sync, `-prune=550` (and everything else) working as expected.
```
759M /home/james/tmp/bitcoin-au-testing/blocks
total 650M
drwx------ 3 james users 4.0K May 31 10:54 .
drwx------ 5 james users 4.0K May 31 11:09 ..
-rw------- 1 james users 127M May 29 15:33 blk03626.dat
-rw-------
...
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1570432290)
Rebased
Also fixed the minor issues with the `test_utxo_snapshots.sh` demo script.
Since last push, I've completed a full test of the mainnet snapshot; after a few days having finished the background sync, `-prune=550` (and everything else) working as expected.
```
759M /home/james/tmp/bitcoin-au-testing/blocks
total 650M
drwx------ 3 james users 4.0K May 31 10:54 .
drwx------ 5 james users 4.0K May 31 11:09 ..
-rw------- 1 james users 127M May 29 15:33 blk03626.dat
-rw-------
...
💬 Sjors commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570438683)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570438683)
Concept ACK
💬 mzumsande commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570459426)
I'd definitely like this eventually, but I'm not sure how much sense it makes to have it before Erlay is functional (unfortunately, there hasn't been much progress lately in #26283, and even after this is merged Erlay still won't be functional yet).
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570459426)
I'd definitely like this eventually, but I'm not sure how much sense it makes to have it before Erlay is functional (unfortunately, there hasn't been much progress lately in #26283, and even after this is merged Erlay still won't be functional yet).
💬 furszy commented on pull request "wallet: finish addressbook encapsulation and simplify addressbook migration":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1570466415)
rebased
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1570466415)
rebased
💬 Sjors commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570469083)
@mzumsande given that we have `-txreconciliation` I think it's fine to add this. We can always drop it if Erlay is given up on. Making it easier to test, makes it more likely to ever get finished though.
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570469083)
@mzumsande given that we have `-txreconciliation` I think it's fine to add this. We can always drop it if Erlay is given up on. Making it easier to test, makes it more likely to ever get finished though.
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570475171)
> One alternative could be to avoid the need to process arguments on the client side at all and and add a new `exec` RPC method that takes an `args` array of strings and an optional `stdin` string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An `exec` method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new `-exec` or `-dumb` option to invok
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570475171)
> One alternative could be to avoid the need to process arguments on the client side at all and and add a new `exec` RPC method that takes an `args` array of strings and an optional `stdin` string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An `exec` method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new `-exec` or `-dumb` option to invok
...
💬 hebasto commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1570487179)
> just need to decide out what to do about guix.
What options are currently being considered. Is an upstream patch still possible?
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1570487179)
> just need to decide out what to do about guix.
What options are currently being considered. Is an upstream patch still possible?
💬 mzumsande commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570503893)
> given that we have `-txreconciliation` I think it's fine to add this.
But `-txreconciliation` is off-by default and not exposed to users, this field exposes Erlay-related info unconditionally in a popular RPC, which could create confusion (why is this field there if I can't turn Erlay on yet?) and seems premature given that Erlay could still be years away (if it makes it at all, which I do hope!). So I'd prefer it if these commits were part of #21515 (which anyone wanting to actually test E
...
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570503893)
> given that we have `-txreconciliation` I think it's fine to add this.
But `-txreconciliation` is off-by default and not exposed to users, this field exposes Erlay-related info unconditionally in a popular RPC, which could create confusion (why is this field there if I can't turn Erlay on yet?) and seems premature given that Erlay could still be years away (if it makes it at all, which I do hope!). So I'd prefer it if these commits were part of #21515 (which anyone wanting to actually test E
...
💬 brunoerg commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570544534)
I implemented this for testing purposes which have been very useful for me and could be for other reviewers, initially my idea is not open this, just did it after seeing https://github.com/bitcoin/bitcoin/issues/26602. However, I agree with @mzumsande about annoyability of removing fields from RPC, so I can leave this as "draft" for more discussions/not being merged and hopefully these commits can be picked in #21515 and then we can have it.
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570544534)
I implemented this for testing purposes which have been very useful for me and could be for other reviewers, initially my idea is not open this, just did it after seeing https://github.com/bitcoin/bitcoin/issues/26602. However, I agree with @mzumsande about annoyability of removing fields from RPC, so I can leave this as "draft" for more discussions/not being merged and hopefully these commits can be picked in #21515 and then we can have it.
📝 brunoerg converted_to_draft a pull request: "rpc, net: add erlay status in `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/27797)
Fixes #26602
Adds `m_tx_reconciliation` in `Peer` struct
to know whether the peer supports Erlay and
exposes it in `getpeerinfo` rpc.
(https://github.com/bitcoin/bitcoin/pull/27797)
Fixes #26602
Adds `m_tx_reconciliation` in `Peer` struct
to know whether the peer supports Erlay and
exposes it in `getpeerinfo` rpc.
🤔 zkfrio requested changes to a pull request: "wallet: Deniability API (Unilateral Transaction Meta-Privacy)"
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1453686280)
NACK
It doesn't improve privacy. Not considering fingerprints. A better way to improve privacy in bitcoin payments using bitcoin core could be payjoin.
False sense of privacy implemented in this pull request only helps miners with fees.
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1453686280)
NACK
It doesn't improve privacy. Not considering fingerprints. A better way to improve privacy in bitcoin payments using bitcoin core could be payjoin.
False sense of privacy implemented in this pull request only helps miners with fees.
💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570556203)
> The commits can be improved, but currently good enough for (non-guix) testing / discussion.
What are plans regarding Guix builds?
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570556203)
> The commits can be improved, but currently good enough for (non-guix) testing / discussion.
What are plans regarding Guix builds?
💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570559259)
> What are plans regarding Guix builds?
What you've quoted was just an outdated part of the PR description, that I've not dropped.
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570559259)
> What are plans regarding Guix builds?
What you've quoted was just an outdated part of the PR description, that I've not dropped.
💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570563184)
> > What are plans regarding Guix builds?
>
> What you've quoted was just an outdated part of the PR description, that I've now dropped.
Well, then Guix does not produced unsigned `*-apple-darwin-unsigned.zip`, instead of `*-apple-darwin-unsigned.dmg`:
```
1bed850e74f985e3db277160529aff767f4d3be00822ee8e204cdff304664727 guix-build-bec052f9ac0b/output/arm64-apple-darwin/SHA256SUMS.part
6b3a93b33d163dcc6a1a8582c3db1fe2c828620757c078e0c59f3e1b1eee3990 guix-build-bec052f9ac0b/output/arm64
...
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1570563184)
> > What are plans regarding Guix builds?
>
> What you've quoted was just an outdated part of the PR description, that I've now dropped.
Well, then Guix does not produced unsigned `*-apple-darwin-unsigned.zip`, instead of `*-apple-darwin-unsigned.dmg`:
```
1bed850e74f985e3db277160529aff767f4d3be00822ee8e204cdff304664727 guix-build-bec052f9ac0b/output/arm64-apple-darwin/SHA256SUMS.part
6b3a93b33d163dcc6a1a8582c3db1fe2c828620757c078e0c59f3e1b1eee3990 guix-build-bec052f9ac0b/output/arm64
...
💬 Sjors commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1570566067)
The first two commits up to f50fa248f1d904be5f33a29c837f74a2cca8abc0 look good to me.
How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
> Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
In any case moving `AcceptBlock` from `Chainstate` to `ChainstateManager` seems an improvement.
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1570566067)
The first two commits up to f50fa248f1d904be5f33a29c837f74a2cca8abc0 look good to me.
How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
> Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.
In any case moving `AcceptBlock` from `Chainstate` to `ChainstateManager` seems an improvement.
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1212012110)
Removed it, thanks!
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1212012110)
Removed it, thanks!
💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1570569762)
Force-pushed removing the unused variable `random_mutable_transaction`
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1570569762)
Force-pushed removing the unused variable `random_mutable_transaction`
💬 hebasto commented on pull request "Fix transaction view/table":
(https://github.com/bitcoin-core/gui/pull/662#issuecomment-1570569786)
Closing due to a long period of inactivity here. Feel free to reopen.
(https://github.com/bitcoin-core/gui/pull/662#issuecomment-1570569786)
Closing due to a long period of inactivity here. Feel free to reopen.
✅ hebasto closed a pull request: "Fix transaction view/table"
(https://github.com/bitcoin-core/gui/pull/662)
(https://github.com/bitcoin-core/gui/pull/662)