π fanquake merged a pull request: "[25.x] Parallel compact block downloads"
(https://github.com/bitcoin/bitcoin/pull/27752)
(https://github.com/bitcoin/bitcoin/pull/27752)
π¬ glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1620036644)
Thanks everyone for the reviews!
Last push:
- Implemented changes that were discussed on the BIP PR https://github.com/bitcoin/bips/pull/1382
- Maximum of 100 transactions per `getpkgtxns` request
- Changed `versions` field in `sendpackages` to be 64b instead of 32b.
- Added a `PKG_RELAY_PKGTXNS` bit for negotiating `getpkgtxns`/`pkgtxns` support in `sendpackages`.
- Fixed some of the bigger problems I found / pointed out by reviewers, e.g.
- https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1620036644)
Thanks everyone for the reviews!
Last push:
- Implemented changes that were discussed on the BIP PR https://github.com/bitcoin/bips/pull/1382
- Maximum of 100 transactions per `getpkgtxns` request
- Changed `versions` field in `sendpackages` to be 64b instead of 32b.
- Added a `PKG_RELAY_PKGTXNS` bit for negotiating `getpkgtxns`/`pkgtxns` support in `sendpackages`.
- Fixed some of the bigger problems I found / pointed out by reviewers, e.g.
- https://github.com/bitcoin/bit
...
π MarcoFalke opened a pull request: "test: Check expected_stderr after stop"
(https://github.com/bitcoin/bitcoin/pull/28028)
This fixes a bug where stderr wasn't checked for the shutdown sequence.
Fix that by waiting for the shutdown to finish and then check stderr.
(https://github.com/bitcoin/bitcoin/pull/28028)
This fixes a bug where stderr wasn't checked for the shutdown sequence.
Fix that by waiting for the shutdown to finish and then check stderr.
π¬ ItIsOHM commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1620202748)
> The description for "data" is wrong, copy-pasting from another (unrelated) data field is not a good approach here. I would suggest digging into the code to understand what "data" is used for, and/or reading the spec of [BIP22](https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki) and [BIP23](https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki)
Oh yes I agree, I'll read more about it and change the descriptions. I was asking in terms of syntax and usage, if it was correc
...
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1620202748)
> The description for "data" is wrong, copy-pasting from another (unrelated) data field is not a good approach here. I would suggest digging into the code to understand what "data" is used for, and/or reading the spec of [BIP22](https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki) and [BIP23](https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki)
Oh yes I agree, I'll read more about it and change the descriptions. I was asking in terms of syntax and usage, if it was correc
...
π furszy approved a pull request: "Remove confusing "Dust" label from coincontrol / sendcoins dialog"
(https://github.com/bitcoin-core/gui/pull/719#pullrequestreview-1512893564)
ACK a582b41
(https://github.com/bitcoin-core/gui/pull/719#pullrequestreview-1512893564)
ACK a582b41
π¬ stickies-v commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1620285061)
Yeah the approach is good. Great, have a look and then for further implementation feedback I'd suggest you open a PR with the code changes (definitely worth going through [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) first).
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1620285061)
Yeah the approach is good. Great, have a look and then for further implementation feedback I'd suggest you open a PR with the code changes (definitely worth going through [CONTRIBUTING.md](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) first).
π¬ Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1620356102)
Thanks for improving these tests. Added to review list.
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1620356102)
Thanks for improving these tests. Added to review list.
π¬ t-bast commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1620368501)
> Iβve fixed a number of issues, if this latest pushβs checks turn up green, it might be more fruitful to try now.
I confirm that, I've ran my set of tests against [eclair](https://github.com/ACINQ/eclair) and everything looks good using https://github.com/bitcoin/bitcoin/pull/26152/commits/0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 :tada:
The tests I ran can be found in the last two commits of [this branch](https://github.com/ACINQ/eclair/commits/wip-test-ancestor-aware-funding-bitcoind-25
...
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1620368501)
> Iβve fixed a number of issues, if this latest pushβs checks turn up green, it might be more fruitful to try now.
I confirm that, I've ran my set of tests against [eclair](https://github.com/ACINQ/eclair) and everything looks good using https://github.com/bitcoin/bitcoin/pull/26152/commits/0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 :tada:
The tests I ran can be found in the last two commits of [this branch](https://github.com/ACINQ/eclair/commits/wip-test-ancestor-aware-funding-bitcoind-25
...
π¬ glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620375694)
> I don't think you need to special case individual transactions even, actually. Instead, use this, instead of chunking:
I'm not sure about using a group's aggregate feerate without checking their spending relationships, as it may allow unrelated transactions to pay for each other. For example:
```
A(1) B(3)
^ ^
C(100)
```
Where minfeerate is 2sat/vB. Imagine C is invalid (e.g. a fake child created to connect A and B).
The ancestor score-based linearization I was imagining (i
...
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620375694)
> I don't think you need to special case individual transactions even, actually. Instead, use this, instead of chunking:
I'm not sure about using a group's aggregate feerate without checking their spending relationships, as it may allow unrelated transactions to pay for each other. For example:
```
A(1) B(3)
^ ^
C(100)
```
Where minfeerate is 2sat/vB. Imagine C is invalid (e.g. a fake child created to connect A and B).
The ancestor score-based linearization I was imagining (i
...
π¬ instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1252120078)
> download should be dedup when we receive the ancpkginfos ?
If it's in your orphanage it won't be fetched again. See `ReceivedAncPkgInfo`
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1252120078)
> download should be dedup when we receive the ancpkginfos ?
If it's in your orphanage it won't be fetched again. See `ReceivedAncPkgInfo`
π¬ instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620395050)
Nothing is perfect, but a linearizer would ideally pick `B` before `A`, yes. So you might get:
`B, A, C`
or
`B, C, A`
depending on if the strategy is greedy. "topo" sort may miss this, which is why we should probably be smarter than that?
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620395050)
Nothing is perfect, but a linearizer would ideally pick `B` before `A`, yes. So you might get:
`B, A, C`
or
`B, C, A`
depending on if the strategy is greedy. "topo" sort may miss this, which is why we should probably be smarter than that?
π hebasto approved a pull request: "Remove confusing "Dust" label from coincontrol / sendcoins dialog"
(https://github.com/bitcoin-core/gui/pull/719#pullrequestreview-1513056670)
Looks good. ACK a582b4141f0756faa3793fb1c772898a984c83e4.
(https://github.com/bitcoin-core/gui/pull/719#pullrequestreview-1513056670)
Looks good. ACK a582b4141f0756faa3793fb1c772898a984c83e4.
π¬ sipa commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620413203)
@glozow You're right; I didn't think this through.
@instagibbs To a limited extent better linearization can help here (though within-chunk optimization isn't something I've been looking at, as it doesn't matter for mining/eviction) but I think you can construct more complex examples where even a "perfect" linearization results in grouping of things that should not pay for each other.
I'm starting to think that something closer to your idea here is right: trying ancestor sets of every transacti
...
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620413203)
@glozow You're right; I didn't think this through.
@instagibbs To a limited extent better linearization can help here (though within-chunk optimization isn't something I've been looking at, as it doesn't matter for mining/eviction) but I think you can construct more complex examples where even a "perfect" linearization results in grouping of things that should not pay for each other.
I'm starting to think that something closer to your idea here is right: trying ancestor sets of every transacti
...
β
hebasto closed an issue: "Confusing/misleading "Dust:" label in coin selection dialog"
(https://github.com/bitcoin-core/gui/issues/699)
(https://github.com/bitcoin-core/gui/issues/699)
π hebasto merged a pull request: "Remove confusing "Dust" label from coincontrol / sendcoins dialog"
(https://github.com/bitcoin-core/gui/pull/719)
(https://github.com/bitcoin-core/gui/pull/719)
π¬ luke-jr commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#discussion_r1252148488)
Won't this lose the other `CFLAGS` currently passed by the user?
(https://github.com/bitcoin/bitcoin/pull/27991#discussion_r1252148488)
Won't this lose the other `CFLAGS` currently passed by the user?
π¬ luke-jr commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1620432636)
If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds...
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1620432636)
If both are provided, I would expect both to be active in parallel, and complete even if the other succeeds...
π€ luke-jr requested changes to a pull request: "init: adding check for : for -torcontrol flag"
(https://github.com/bitcoin/bitcoin/pull/28018#pullrequestreview-1513100420)
Concept NACK. Port isn't required (the default is 9051). Furthermore, even "1" is a valid value (it would be the same as 0.0.0.1:9051), though I suppose it wouldn't hurt to check for it specifically and reject it.
(https://github.com/bitcoin/bitcoin/pull/28018#pullrequestreview-1513100420)
Concept NACK. Port isn't required (the default is 9051). Furthermore, even "1" is a valid value (it would be the same as 0.0.0.1:9051), though I suppose it wouldn't hurt to check for it specifically and reject it.
π¬ luke-jr commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252163957)
`rfind` is a very weird choice here. Suggest using [`compare`](https://cplusplus.com/reference/string/string/compare/)
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252163957)
`rfind` is a very weird choice here. Suggest using [`compare`](https://cplusplus.com/reference/string/string/compare/)
π¬ Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1620448305)
Building 8587bfbbbc33f2b5e3737c9afe42e6d09464064c fails with:
```
wallet/wallet.cpp: In function βstd::shared_ptr<wallet::CWallet> wallet::CreateWallet(WalletContext&, const std::string&, std::optional<bool>, DatabaseOptions&, DatabaseStatus&, bilingual_str&, std::vector<bilingual_str>&)β:
wallet/wallet.cpp:342:34: error: βWALLET_FLAG_GLOBAL_HD_KEYβ was not declared in this scope
342 | wallet_creation_flags |= WALLET_FLAG_GLOBAL_HD_KEY;
| ^
...
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1620448305)
Building 8587bfbbbc33f2b5e3737c9afe42e6d09464064c fails with:
```
wallet/wallet.cpp: In function βstd::shared_ptr<wallet::CWallet> wallet::CreateWallet(WalletContext&, const std::string&, std::optional<bool>, DatabaseOptions&, DatabaseStatus&, bilingual_str&, std::vector<bilingual_str>&)β:
wallet/wallet.cpp:342:34: error: βWALLET_FLAG_GLOBAL_HD_KEYβ was not declared in this scope
342 | wallet_creation_flags |= WALLET_FLAG_GLOBAL_HD_KEY;
| ^
...