💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074262054)
`CBlockIndexWorkComparator()(a, b))` means that a is strictly smaller than b.
`!CBlockIndexWorkComparator()(b, a))` means that a is smaller or or equal to b (in this case, equal means the same block index because of the [pointer address criterion](https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/node/blockstorage.cpp#L165-L168)).
I think that's why sometimes one or the other is used.
In this case, it doesn't really matter because the two elements are ch
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074262054)
`CBlockIndexWorkComparator()(a, b))` means that a is strictly smaller than b.
`!CBlockIndexWorkComparator()(b, a))` means that a is smaller or or equal to b (in this case, equal means the same block index because of the [pointer address criterion](https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/node/blockstorage.cpp#L165-L168)).
I think that's why sometimes one or the other is used.
In this case, it doesn't really matter because the two elements are ch
...
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074262125)
done
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074262125)
done
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074262220)
done
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074262220)
done
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074264089)
done. The suggested comment is a bit too strong for my taste: In reality the distinction doesn't matter, even if both were set nothing bad would happen, see #31835. This is more for being consistent with how it's handled elsewhere.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074264089)
done. The suggested comment is a bit too strong for my taste: In reality the distinction doesn't matter, even if both were set nothing bad would happen, see #31835. This is more for being consistent with how it's handled elsewhere.
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074264281)
done, good idea!
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074264281)
done, good idea!
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074264411)
done
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074264411)
done
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074270206)
Hmm, this PR doesn't really change the essence of this:
The critical thing here is to set `nStatus` to `BLOCK_FAILED_VALID`, and this is already was the case on master.
This is irrevocable (outside of the `reconsiderblock` RPC which is a hack that doesn't really count) and it means that this block - and therefore all of its descendants - will never be connected. If there was a bug (e.g. if we could fail with `BLOCK_TIME_FUTURE` after accepting a header and then receiving the full block) this w
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074270206)
Hmm, this PR doesn't really change the essence of this:
The critical thing here is to set `nStatus` to `BLOCK_FAILED_VALID`, and this is already was the case on master.
This is irrevocable (outside of the `reconsiderblock` RPC which is a hack that doesn't really count) and it means that this block - and therefore all of its descendants - will never be connected. If there was a bug (e.g. if we could fail with `BLOCK_TIME_FUTURE` after accepting a header and then receiving the full block) this w
...
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074344072)
> If that's correct, then I think:
Yes, that's correct.
> CheckBlockIndex() should be called right before releasing, but while it still has the cs_main lock, to avoid synchronization issues (effectively making CheckBlockIndex() an EXCLUSIVE_LOCKS_REQUIRED(::cs_main) method)
`CheckBlockIndex()` is called from multiple places in validation, some of which hold `cs_main`, some of which don't.
My understanding is that it must always pass if `cs_main` isn't held (because at that time, other
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2074344072)
> If that's correct, then I think:
Yes, that's correct.
> CheckBlockIndex() should be called right before releasing, but while it still has the cs_main lock, to avoid synchronization issues (effectively making CheckBlockIndex() an EXCLUSIVE_LOCKS_REQUIRED(::cs_main) method)
`CheckBlockIndex()` is called from multiple places in validation, some of which hold `cs_main`, some of which don't.
My understanding is that it must always pass if `cs_main` isn't held (because at that time, other
...
🤔 achow101 reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2816355060)
Needs a release note.
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2816355060)
Needs a release note.
💬 achow101 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2074352961)
In 34c3ef7160a3e54980d74426f12237a2a674e0f2 "datacarrier: deprecate startup arguments for future removal"
Perhaps this should mention that multiple outputs are allowed?
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2074352961)
In 34c3ef7160a3e54980d74426f12237a2a674e0f2 "datacarrier: deprecate startup arguments for future removal"
Perhaps this should mention that multiple outputs are allowed?
💬 Tech1k commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2852636062)
NACK. While I agree that this change can be beneficial by allowing data-heavy applications consolidate what would have been dust outputs into a single OP_RETURN script, I don’t support removing the arbitrary limit entirely. Instead, I’d favor towards increasing the limit or enabling OP_RETURN chaining, provided appropriate fees are enforced as this change may lead to increased spam.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2852636062)
NACK. While I agree that this change can be beneficial by allowing data-heavy applications consolidate what would have been dust outputs into a single OP_RETURN script, I don’t support removing the arbitrary limit entirely. Instead, I’d favor towards increasing the limit or enabling OP_RETURN chaining, provided appropriate fees are enforced as this change may lead to increased spam.
💬 andrewtoth commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074362360)
I don't think we should add an assert here to explain. The comment right above says `Create the coin in the parent cache`.
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2074362360)
I don't think we should add an assert here to explain. The comment right above says `Create the coin in the parent cache`.
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852675818)
> I don't see why the first commit is necessary. Couldn't you just mock the time so that it's fixed and the number of derivation rounds always stays at the default value?
Maybe I'm not thinking creatively enough, but I think because the timing takes place entirely inside of `EncryptWallet()->EncryptMasterKey`, from the `wallet_encrypt.cpp` benchmark, I could only mock the clock so that it's static during the derivation "benchmark"[^1] runs, so the difference between the end time and the start
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852675818)
> I don't see why the first commit is necessary. Couldn't you just mock the time so that it's fixed and the number of derivation rounds always stays at the default value?
Maybe I'm not thinking creatively enough, but I think because the timing takes place entirely inside of `EncryptWallet()->EncryptMasterKey`, from the `wallet_encrypt.cpp` benchmark, I could only mock the clock so that it's static during the derivation "benchmark"[^1] runs, so the difference between the end time and the start
...
💬 kevkevinpal commented on pull request "fuzz: Remove unused TimeoutExpired catch in fuzz runner":
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2852734496)
crACK [fa48040](https://github.com/bitcoin/bitcoin/pull/32388/commits/fa4804009ceba96926edd7f55ea22442ebdc665d)
Agreed the error message would be wrong but also not needed anymore
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2852734496)
crACK [fa48040](https://github.com/bitcoin/bitcoin/pull/32388/commits/fa4804009ceba96926edd7f55ea22442ebdc665d)
Agreed the error message would be wrong but also not needed anymore
💬 furszy commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852755475)
> so the difference between the end time and the start time of a run will be 0 which will result in dividing by 0:
Yes, it seems much more straightforward to address that inside the encryption function (which could arguably be considered a 'fix', since we shouldn’t be dividing by zero regardless of whether the clock time is messed up) than to add a test-only field to the wallet and another function arg just just for this.
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852755475)
> so the difference between the end time and the start time of a run will be 0 which will result in dividing by 0:
Yes, it seems much more straightforward to address that inside the encryption function (which could arguably be considered a 'fix', since we shouldn’t be dividing by zero regardless of whether the clock time is messed up) than to add a test-only field to the wallet and another function arg just just for this.
💬 petertodd commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2852767981)
ACK 664ae315f4965ef39d333dc915c034fd6181c8aa
Manually tested that the `-datacarriersize` limit does in fact match on a two OP_Return output transaction of the expected size, and that the resulting transaction was propagated to other nodes as expected.
Of course, I still (weakly) prefer my version. But this is acceptable too.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2852767981)
ACK 664ae315f4965ef39d333dc915c034fd6181c8aa
Manually tested that the `-datacarriersize` limit does in fact match on a two OP_Return output transaction of the expected size, and that the resulting transaction was propagated to other nodes as expected.
Of course, I still (weakly) prefer my version. But this is acceptable too.
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852807611)
> since we shouldn’t be dividing by zero regardless of whether the clock time is messed up
AFAIK, `std::chrono::steady_clock`, unlike the system clock, cannot fail to move forward, and this will never happen.
> than to add a test-only field to the wallet and another function arg just just for this.
I agree that adding a test-only field and arg is very bad, and would like to avoid it, but it seems to me worse to imbue secret meaning into the benchmark by handling a time difference of 0
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2852807611)
> since we shouldn’t be dividing by zero regardless of whether the clock time is messed up
AFAIK, `std::chrono::steady_clock`, unlike the system clock, cannot fail to move forward, and this will never happen.
> than to add a test-only field to the wallet and another function arg just just for this.
I agree that adding a test-only field and arg is very bad, and would like to avoid it, but it seems to me worse to imbue secret meaning into the benchmark by handling a time difference of 0
...
🤔 shahsb reviewed a pull request: "miner: don't needlessly append dummy OP_0 to bip34"
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-2816730864)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-2816730864)
Concept ACK
💬 bubelov commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853205353)
The mailing list discussion vastly overestimates VC. If they succeed, they could raise even more money for further attacks, because it will be seen as a proof of their ability to influence the reference implementation. However, if the general sentiment remains hostile, securing funds for abusive blockchain use will become much harder.
Concept NACK.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2853205353)
The mailing list discussion vastly overestimates VC. If they succeed, they could raise even more money for further attacks, because it will be seen as a proof of their ability to influence the reference implementation. However, if the general sentiment remains hostile, securing funds for abusive blockchain use will become much harder.
Concept NACK.
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2853240905)
Refactored to address @furszy feedback to avoid adding a test-only parameter to `CWallet`, and added a somewhat opinionated refactor (https://github.com/bitcoin/bitcoin/pull/31774/commits/7176b26cde7cbaffdd92af9c25f85f8e5233e78a) of `CWallet::EncryptMasterKey()` since I was touching the iteration measuring stuff anyways. I don't like the idea of using a `for` loop that iterates once with `0` and once with `1`, but it's nearly 50% the LOC, I believe it is easier to understand how the weighted ave
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2853240905)
Refactored to address @furszy feedback to avoid adding a test-only parameter to `CWallet`, and added a somewhat opinionated refactor (https://github.com/bitcoin/bitcoin/pull/31774/commits/7176b26cde7cbaffdd92af9c25f85f8e5233e78a) of `CWallet::EncryptMasterKey()` since I was touching the iteration measuring stuff anyways. I don't like the idea of using a `for` loop that iterates once with `0` and once with `1`, but it's nearly 50% the LOC, I believe it is easier to understand how the weighted ave
...