👍 rkrux approved a pull request: "Fee Estimation: Ignore all transactions with an in-block child"
(https://github.com/bitcoin/bitcoin/pull/30079#pullrequestreview-2054547040)
tACK [c12a677](https://github.com/bitcoin/bitcoin/pull/30079/commits/c12a677cc250608171bc4f6311095b60ba24abab)
Make successful, all functional tests successful.
Mentioned few nits and asked few questions for my clarity.
> The downside of this approach is that transactions that are not CPFP’d will aslo be ignored.
Related to this^, one specific case I can think of that will be affected is of layered transactions that are meant to be confirmed together, example: fanout transactions to incr
...
(https://github.com/bitcoin/bitcoin/pull/30079#pullrequestreview-2054547040)
tACK [c12a677](https://github.com/bitcoin/bitcoin/pull/30079/commits/c12a677cc250608171bc4f6311095b60ba24abab)
Make successful, all functional tests successful.
Mentioned few nits and asked few questions for my clarity.
> The downside of this approach is that transactions that are not CPFP’d will aslo be ignored.
Related to this^, one specific case I can think of that will be affected is of layered transactions that are meant to be confirmed together, example: fanout transactions to incr
...
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599579906)
I've seen this function call in other functional tests as well, can you share why this call is required?
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599579906)
I've seen this function call in other functional tests as well, can you share why this call is required?
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599589458)
Nit: The meaning would be more explicit if we called the last argument as `feerate_satvb`.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599589458)
Nit: The meaning would be more explicit if we called the last argument as `feerate_satvb`.
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599579062)
This is an easy candidate to remove code duplication by extracting this out in an internal function, and by accepting `tx` and `feeRate` as function args.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599579062)
This is an easy candidate to remove code duplication by extracting this out in an internal function, and by accepting `tx` and `feeRate` as function args.
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599597279)
Nit: This code block can be extracted in a separate function because after reading the code above that's mostly delegation consecutive function calls, it'd be nice to have something similar here as well - `removeParentTxs`. One more point being that `seen_transactions` variable is internal to this logic and doesn't need to be exposed to `processBlock`.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599597279)
Nit: This code block can be extracted in a separate function because after reading the code above that's mostly delegation consecutive function calls, it'd be nice to have something similar here as well - `removeParentTxs`. One more point being that `seen_transactions` variable is internal to this logic and doesn't need to be exposed to `processBlock`.
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599598936)
I'm assuming the transactions are sorted from parents to children. So in the case A -> B -> C -> D, all except D would be removed, right?
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599598936)
I'm assuming the transactions are sorted from parents to children. So in the case A -> B -> C -> D, all except D would be removed, right?
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599548506)
Nit:
```
[low_feerate, med_feerate, high_feerate] = [Decimal(2), Decimal(15), Decimal(20)]
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599548506)
Nit:
```
[low_feerate, med_feerate, high_feerate] = [Decimal(2), Decimal(15), Decimal(20)]
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599591670)
I dont fully understand this conversion, is the intention to convert it to `feerate_satkvb`?
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599591670)
I dont fully understand this conversion, is the intention to convert it to `feerate_satkvb`?
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599613474)
+1 for expressive node names!
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599613474)
+1 for expressive node names!
👍 Sjors approved a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2054489923)
tACK 06c2c713c52b60231efc3e00d2c5eb0bf9e345f9
There's a bunch of things here and in earlier comments that warrant followup, but not worth losing ACKs.
We currently have 24729 blocks and the difficulty is 16,777,216. That makes sense: we've had 12 retarget periods, and if each time the difficulty maximally increased, you get exactly 4^12.
In order to get test vectors in early, we'd have to reset with a new genesis block and immediately publish the transactions.
It would be nice to set
...
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2054489923)
tACK 06c2c713c52b60231efc3e00d2c5eb0bf9e345f9
There's a bunch of things here and in earlier comments that warrant followup, but not worth losing ACKs.
We currently have 24729 blocks and the difficulty is 16,777,216. That makes sense: we've had 12 retarget periods, and if each time the difficulty maximally increased, you get exactly 4^12.
In order to get test vectors in early, we'd have to reset with a new genesis block and immediately publish the transactions.
It would be nice to set
...
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599541265)
06c2c713c52b60231efc3e00d2c5eb0bf9e345f9: Suggested comment:
```cpp
// For the first block of each difficulty adjustment interval,
// except the genesis block.
```
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599541265)
06c2c713c52b60231efc3e00d2c5eb0bf9e345f9: Suggested comment:
```cpp
// For the first block of each difficulty adjustment interval,
// except the genesis block.
```
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599579980)
06c2c713c52b60231efc3e00d2c5eb0bf9e345f9: I initially thought that this should be moved to `ConnectBlock`, somewhere in the "Sanity checks", maybe right before `bool fScriptChecks = true;`. But it's fine here.
It doesn't _need_ to be here, because we don't use the system clock for this check. However it's more readable to have it right next to the `time-too-old` check. So is that safe?
`ContextualCheckBlockHeader` is only called when we receive a header. It's called by `AcceptBlockHeader`
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599579980)
06c2c713c52b60231efc3e00d2c5eb0bf9e345f9: I initially thought that this should be moved to `ConnectBlock`, somewhere in the "Sanity checks", maybe right before `bool fScriptChecks = true;`. But it's fine here.
It doesn't _need_ to be here, because we don't use the system clock for this check. However it's more readable to have it right next to the `time-too-old` check. So is that safe?
`ContextualCheckBlockHeader` is only called when we receive a header. It's called by `AcceptBlockHeader`
...
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599611622)
e172a96c0781de2bbd69312905d2c16cc1745c2f: `const uint32_t`? (matching return type of `GetCompact()`)
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599611622)
e172a96c0781de2bbd69312905d2c16cc1745c2f: `const uint32_t`? (matching return type of `GetCompact()`)
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599508314)
06c2c713c52b60231efc3e00d2c5eb0bf9e345f9: we have to remember to update this hardcoded value in three different places if we reset testnet4 (before the release). I verified that it currently matches the actual genesis block (`getblockhash 0`).
It's not dangerous to mainnet if we forget, it would only break testnet4. But I would prefer to introduce an `enum class Network` so we can do `consensusParams.network == Network::Testnet4`. It can wait for a followup.
If you're worried about growing
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599508314)
06c2c713c52b60231efc3e00d2c5eb0bf9e345f9: we have to remember to update this hardcoded value in three different places if we reset testnet4 (before the release). I verified that it currently matches the actual genesis block (`getblockhash 0`).
It's not dangerous to mainnet if we forget, it would only break testnet4. But I would prefer to introduce an `enum class Network` so we can do `consensusParams.network == Network::Testnet4`. It can wait for a followup.
If you're worried about growing
...
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599609418)
e172a96c0781de2bbd69312905d2c16cc1745c2f: `params.fPowAllowMinDifficultyBlocks &&` is redundant. Though you could an assert.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599609418)
e172a96c0781de2bbd69312905d2c16cc1745c2f: `params.fPowAllowMinDifficultyBlocks &&` is redundant. Though you could an assert.
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599635493)
e172a96c0781de2bbd69312905d2c16cc1745c2f: this had me confused again, so maybe a comment is helpful.
If miners go away in the middle of difficulty adjustment period A, this rule will find the last "real" `nBits` value and apply it to the first block of period B. If nothing changes, then for period C this seeks back to the first block of period B, finds the real value and applies it.
But then how do the first blocks of B and C ever mined?
Well, remember that `GetNextWorkRequired` ignore
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599635493)
e172a96c0781de2bbd69312905d2c16cc1745c2f: this had me confused again, so maybe a comment is helpful.
If miners go away in the middle of difficulty adjustment period A, this rule will find the last "real" `nBits` value and apply it to the first block of period B. If nothing changes, then for period C this seeks back to the first block of period B, finds the real value and applies it.
But then how do the first blocks of B and C ever mined?
Well, remember that `GetNextWorkRequired` ignore
...
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599648112)
@russeree the use case for these test vectors is for regression testing, so you'd want to spin up a testnet4 node, sync a few thousand blocks and delete it again. Having such a small number of blocks might allow you to store the actual blocks on the CI server - so it doesn't need to make network requests.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1599648112)
@russeree the use case for these test vectors is for regression testing, so you'd want to spin up a testnet4 node, sync a few thousand blocks and delete it again. Having such a small number of blocks might allow you to store the actual blocks on the CI server - so it doesn't need to make network requests.
💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599652146)
> I'm assuming the transactions are sorted from parents to children.
Yes, they are.
The fee estimator does not take all transaction with parents into account already see
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L615
Assuming A->B->C->D are topologically sorted from parent to descendant.
In this case B, C, D are already not considered for fee estimation, After this PR A will also be ignored.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599652146)
> I'm assuming the transactions are sorted from parents to children.
Yes, they are.
The fee estimator does not take all transaction with parents into account already see
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L615
Assuming A->B->C->D are topologically sorted from parent to descendant.
In this case B, C, D are already not considered for fee estimation, After this PR A will also be ignored.
👍 TheCharlatan approved a pull request: "refactor: simplify `FormatSubVersion` using strprintf/Join"
(https://github.com/bitcoin/bitcoin/pull/30098#pullrequestreview-2054780864)
tACK 12d82817bf32396b58c8c65645012def606680b6
(https://github.com/bitcoin/bitcoin/pull/30098#pullrequestreview-2054780864)
tACK 12d82817bf32396b58c8c65645012def606680b6
🤔 glozow reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2054751541)
Thanks @mzumsande @AngusP @instagibbs! Addressed comments. I've also expanded the "cast from txid to wtxid" part to be slightly more verbose but hopefully easier to understand.
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2054751541)
Thanks @mzumsande @AngusP @instagibbs! Addressed comments. I've also expanded the "cast from txid to wtxid" part to be slightly more verbose but hopefully easier to understand.