💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214720397)
If I understand correctly the rate-limiting accounting introduced here, whatever the number of orphan parent missing, there will be an in-flight orphan parent requests issued (i.e if we’re missing 10 parents, we’re going to issue 10 in-flight orphan parent requests).
I think this should be documented as multi-party applications and contracting protocols transactions issuers might have to assemble their packages in function.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214720397)
If I understand correctly the rate-limiting accounting introduced here, whatever the number of orphan parent missing, there will be an in-flight orphan parent requests issued (i.e if we’re missing 10 parents, we’re going to issue 10 in-flight orphan parent requests).
I think this should be documented as multi-party applications and contracting protocols transactions issuers might have to assemble their packages in function.
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214712117)
There is a argument that can be made if memory and throughputs limits (e.g `MAX_PEER_ANNOUNCEMENTS`) are applied in function of the type of transactions received (e.g v3 transactions) and that can be used to nudge multi-party and contracting protocols transactions broadcasters to use fee-bumping efficient mechanism such as one CPFP covering multiple parents due to the package limits. This would be a discount incentive by analogy with SegWit spending discounted by consensus validation rules.
O
...
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214712117)
There is a argument that can be made if memory and throughputs limits (e.g `MAX_PEER_ANNOUNCEMENTS`) are applied in function of the type of transactions received (e.g v3 transactions) and that can be used to nudge multi-party and contracting protocols transactions broadcasters to use fee-bumping efficient mechanism such as one CPFP covering multiple parents due to the package limits. This would be a discount incentive by analogy with SegWit spending discounted by consensus validation rules.
O
...
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214728153)
This is unclear what level of enforcement is granted to a “final” decision, if the transactions are added to any transaction-relay download filters such as `m_recent_rejects` or `m_recently_announced_invs`. Otherwise, in case of blocks re-orgs dropping out the first package component and a second package component still stuck in filters, this can be source of package propagation failure.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214728153)
This is unclear what level of enforcement is granted to a “final” decision, if the transactions are added to any transaction-relay download filters such as `m_recent_rejects` or `m_recently_announced_invs`. Otherwise, in case of blocks re-orgs dropping out the first package component and a second package component still stuck in filters, this can be source of package propagation failure.
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214732393)
I think there can be a source of concerns if all the announced orphans are second components from a single common parent. All our peers would announce those second-stage components, only one of them will succeed in the mempool validation and other will be refused as conflicts, I think.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214732393)
I think there can be a source of concerns if all the announced orphans are second components from a single common parent. All our peers would announce those second-stage components, only one of them will succeed in the mempool validation and other will be refused as conflicts, I think.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214738570)
(I just realized I answered my own question from https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214675802)
But still, don't we also send automatic PINGs ? Could that result in premature disconnection?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214738570)
(I just realized I answered my own question from https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214675802)
But still, don't we also send automatic PINGs ? Could that result in premature disconnection?
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214751567)
Orphan usage would effect the CPFP transactions, IIUC, not the commitment transactions. The CPFP is deemed high enough, sent to the peer, the peer holds the child tx in orphan set and requests the full package, which isn't size-bound(beyond current mempool limits).
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214751567)
Orphan usage would effect the CPFP transactions, IIUC, not the commitment transactions. The CPFP is deemed high enough, sent to the peer, the peer holds the child tx in orphan set and requests the full package, which isn't size-bound(beyond current mempool limits).
💬 brunoerg commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1214786122)
nit:
```suggestion
if blockmintxfee_btc_kvb > 0: # can't go below zero-fee
```
I think `>` fits better with the comment "can't go below zero-fee" then `!=`
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1214786122)
nit:
```suggestion
if blockmintxfee_btc_kvb > 0: # can't go below zero-fee
```
I think `>` fits better with the comment "can't go below zero-fee" then `!=`
👍 brunoerg approved a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1458443810)
crACK 989fa37670cbb386873a765799fd6db8ea6986f7
Nice test coverage!
Just a thought that came to my mind while reviewing it:
- Seems like these test coverage uses just 1 node, if we'd stop node1 right before line 81, it wouldn't make the test to fail. So, it seems that `-minrelaytxfee=0` is just to make `send_self_transfer` - which calls `sendrawtransaction` - not fail (even if we might not have any connection).
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1458443810)
crACK 989fa37670cbb386873a765799fd6db8ea6986f7
Nice test coverage!
Just a thought that came to my mind while reviewing it:
- Seems like these test coverage uses just 1 node, if we'd stop node1 right before line 81, it wouldn't make the test to fail. So, it seems that `-minrelaytxfee=0` is just to make `send_self_transfer` - which calls `sendrawtransaction` - not fail (even if we might not have any connection).
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1574302283)
Correct me please if I'm missing anything. Considering:
```
- Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.
- After we receive the full transaction as a TX message, in reply to our GETDATA request, only then consider the transaction has propagated through the network and remove it from the unbroadcast list.
```
and
```
We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not t
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1574302283)
Correct me please if I'm missing anything. Considering:
```
- Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.
- After we receive the full transaction as a TX message, in reply to our GETDATA request, only then consider the transaction has propagated through the network and remove it from the unbroadcast list.
```
and
```
We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not t
...
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214831451)
It seems for extra anonymity, but I have a similar doubt, couldn't I make easier for someone to censor us?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214831451)
It seems for extra anonymity, but I have a similar doubt, couldn't I make easier for someone to censor us?
💬 instagibbs commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214832859)
unless we want to sybil the network and fake long-term connections, it's obvious what this connection will be about? Just retry with new peers until someone honest propagates it?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214832859)
unless we want to sybil the network and fake long-term connections, it's obvious what this connection will be about? Just retry with new peers until someone honest propagates it?
💬 ryanofsky commented on pull request "wallet: Add tracing for sqlite statements":
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574316464)
> Turning this on will log private keys as they are being written to the db, which seems a little scary.
Yes when the tracing was turned on, having access to the debug log file would be like having access to the database file. The keys would at least be encrypted if the wallet had a password set, but it could still be scary/unexpected behavior.
I looked into ways of trying to mask values in the sql statement, but the `sqlite3_stmt` object is pretty opaque and I couldn't find any function h
...
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574316464)
> Turning this on will log private keys as they are being written to the db, which seems a little scary.
Yes when the tracing was turned on, having access to the debug log file would be like having access to the database file. The keys would at least be encrypted if the wallet had a password set, but it could still be scary/unexpected behavior.
I looked into ways of trying to mask values in the sql statement, but the `sqlite3_stmt` object is pretty opaque and I couldn't find any function h
...
💬 achow101 commented on pull request "wallet: Add tracing for sqlite statements":
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574364005)
ACK ff9d961bf38b24f8f931dcf66799cbc468e473df
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1574364005)
ACK ff9d961bf38b24f8f931dcf66799cbc468e473df
💬 achow101 commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1214886372)
In bbb927739955b1588d94c5fd8ed73ddf5afde46c "fuzz: Fix mini_miner_selection running out of coin"
Is it possible that `available_coins` is empty and so `num_inputs = 0`? It may be useful to have an additional assert before the inputs are filled below.
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1214886372)
In bbb927739955b1588d94c5fd8ed73ddf5afde46c "fuzz: Fix mini_miner_selection running out of coin"
Is it possible that `available_coins` is empty and so `num_inputs = 0`? It may be useful to have an additional assert before the inputs are filled below.
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214935160)
Thank you, I added the delete attempt
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214935160)
Thank you, I added the delete attempt
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214936512)
Sure, thank you, I updated the test.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214936512)
Sure, thank you, I updated the test.
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1574425436)
> Also wondering if we should include a hidden argument to allow older files for testing reasons, re: #27415
> I've used it for systems I've built for integration testing, and not having to touch a file during testing to refresh it would be a simplification.
>
> > For testing, they could mock the time right?
>
> Depends on your test setup, really. It might interfere during integration tests which require a realistic clock?
>
> Makes sense, seems fine as a regtest-only option for testing
...
(https://github.com/bitcoin/bitcoin/pull/27622#issuecomment-1574425436)
> Also wondering if we should include a hidden argument to allow older files for testing reasons, re: #27415
> I've used it for systems I've built for integration testing, and not having to touch a file during testing to refresh it would be a simplification.
>
> > For testing, they could mock the time right?
>
> Depends on your test setup, really. It might interfere during integration tests which require a realistic clock?
>
> Makes sense, seems fine as a regtest-only option for testing
...
💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1215190508)
I think the answer is just to add a new tx entry field for the steady clock time and use that at natural precision.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1215190508)
I think the answer is just to add a new tx entry field for the steady clock time and use that at natural precision.
💬 desibitcoiner commented on issue "Build broken when enabling fuzzing on Apple M1 hw using homebrew llvm.":
(https://github.com/bitcoin/bitcoin/issues/27550#issuecomment-1574690664)
I'm trying to run a build on Fedora 36 and I'm seeing the same error.
When I run ./configure I see the following:
configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
configure: WARNING: Passing --without-bdb will suppress this warning
checking for SQLITE... yes
checking whether to build wallet with support for sqlite... yes
checking whether Userspace, Statically Defined Tracing tracepoints are supported... no
checking for natpmp.h... yes
checking for
...
(https://github.com/bitcoin/bitcoin/issues/27550#issuecomment-1574690664)
I'm trying to run a build on Fedora 36 and I'm seeing the same error.
When I run ./configure I see the following:
configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
configure: WARNING: Passing --without-bdb will suppress this warning
checking for SQLITE... yes
checking whether to build wallet with support for sqlite... yes
checking whether Userspace, Statically Defined Tracing tracepoints are supported... no
checking for natpmp.h... yes
checking for
...
⚠️ desibitcoiner opened an issue: "Build fails on Fedora 36 - configure failed for src/secp256k1"
(https://github.com/bitcoin/bitcoin/issues/27808)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Found an recent similar issue but didn't help:
https://github.com/bitcoin/bitcoin/issues/27550
I'm trying to run a build on Fedora 36 and I'm seeing the same error.
When I run ./configure I see the following:
configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
configure: WARNING: Passing --without-bdb will suppress this warning
checking for SQLI
...
(https://github.com/bitcoin/bitcoin/issues/27808)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Found an recent similar issue but didn't help:
https://github.com/bitcoin/bitcoin/issues/27550
I'm trying to run a build on Fedora 36 and I'm seeing the same error.
When I run ./configure I see the following:
configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
configure: WARNING: Passing --without-bdb will suppress this warning
checking for SQLI
...