💬 fanquake commented on pull request "crypto: disable asan for sha256_sse4 with clang and -O0":
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2109540189)
Checked this (`./configure --enable-debug --with-sanitizers=address CC=clang CXX=clang++`) works with `Ubuntu clang version 18.1.3 (1)` on x86_64.
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2109540189)
Checked this (`./configure --enable-debug --with-sanitizers=address CC=clang CXX=clang++`) works with `Ubuntu clang version 18.1.3 (1)` on x86_64.
💬 paplorinc commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599578304)
But we're not actually testing the production code this way, i.e. the test would still pass if `NUMS_H_DATA` were to change (e.g. after an accidental local search/replace or whatever).
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599578304)
But we're not actually testing the production code this way, i.e. the test would still pass if `NUMS_H_DATA` were to change (e.g. after an accidental local search/replace or whatever).
💬 theStack commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599582598)
Should add secp256k1 to the unit test modules list (so it also gets executed on CI):
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/test/functional/feature_framework_unit_tests.py#L15-L17
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599582598)
Should add secp256k1 to the unit test modules list (so it also gets executed on CI):
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/test/functional/feature_framework_unit_tests.py#L15-L17
💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2109571959)
> The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalReason class or initialize it in an inconsistent state.
Seems easily addressed with a constructor, no? Something like:
```c++
class RemovedReason {
public:
MemPoolRemovalReason m_reason;
std::variant<std::monostate, CTxRef
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2109571959)
> The drawback of a separate data field is that there is no longer a guarantee that data accompanying the reason is always present when the reason is set, and always absent when it is not set. So it is possible for code to only partially initialize the RemovalReason class or initialize it in an inconsistent state.
Seems easily addressed with a constructor, no? Something like:
```c++
class RemovedReason {
public:
MemPoolRemovalReason m_reason;
std::variant<std::monostate, CTxRef
...
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599597520)
Done!
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1599597520)
Done!
💬 TheCharlatan commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2109589719)
Thanks for the ACKs, should be trivial to re-ACK.
Updated 04ffe4061da2d0135f206032e167703772b5da78 -> d4b17c7d46ad8e2833ade99d5b4c9741c913e84d ([rmKernelBatchPriority_0](https://github.com/TheCharlatan/bitcoin/tree/rmKernelBatchPriority_0) -> [rmKernelBatchPriority_1](https://github.com/TheCharlatan/bitcoin/tree/rmKernelBatchPriority_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/rmKernelBatchPriority_0..rmKernelBatchPriority_1))
* Addressed @ryanofsky's [comment](https://gi
...
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2109589719)
Thanks for the ACKs, should be trivial to re-ACK.
Updated 04ffe4061da2d0135f206032e167703772b5da78 -> d4b17c7d46ad8e2833ade99d5b4c9741c913e84d ([rmKernelBatchPriority_0](https://github.com/TheCharlatan/bitcoin/tree/rmKernelBatchPriority_0) -> [rmKernelBatchPriority_1](https://github.com/TheCharlatan/bitcoin/tree/rmKernelBatchPriority_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/rmKernelBatchPriority_0..rmKernelBatchPriority_1))
* Addressed @ryanofsky's [comment](https://gi
...
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599544002)
This log and the same one on line 520 can go inside `clear_first_node_estimates()`.
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1599544002)
This log and the same one on line 520 can go inside `clear_first_node_estimates()`.
👍 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()`)