💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2557503481)
@optout21 for vision
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2557503481)
@optout21 for vision
🤔 l0rinc requested changes to a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3497639202)
Thank you for considering the previous changes.
Something kept bothering me in the usage of sorted sets, how awkward it was to use them as changing priority queues, so I stepped back and checked why we even need such an optimization in the first place, given don't expect to have millions of entries here.
I have included a benchmark in my suggestions to back my opinion with actual data, please consider it, and feel free to add any part to the current PR if you agree that it simplifies the logic
...
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3497639202)
Thank you for considering the previous changes.
Something kept bothering me in the usage of sorted sets, how awkward it was to use them as changing priority queues, so I stepped back and checked why we even need such an optimization in the first place, given don't expect to have millions of entries here.
I have included a benchmark in my suggestions to back my opinion with actual data, please consider it, and feel free to add any part to the current PR if you agree that it simplifies the logic
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2557559349)
As mentioned above, this hasher is quite heavy, making this a pessimization:
<img width="823" height="411" alt="Image" src="https://github.com/user-attachments/assets/7b8ca62e-6e99-40fd-ad39-5b81e1b02f1c" />
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2557559349)
As mentioned above, this hasher is quite heavy, making this a pessimization:
<img width="823" height="411" alt="Image" src="https://github.com/user-attachments/assets/7b8ca62e-6e99-40fd-ad39-5b81e1b02f1c" />
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556376564)
As the [developer notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes) hint at, this probably isn't fixed in [*16.2*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_2-release-notes) yet, only in [*16.3*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes).
And it seems https://github.com/bitcoin/bitcoin/pull/33932 doesn't change the situation since we're [deliberately downloading](https://git
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556376564)
As the [developer notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes) hint at, this probably isn't fixed in [*16.2*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_2-release-notes) yet, only in [*16.3*](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes).
And it seems https://github.com/bitcoin/bitcoin/pull/33932 doesn't change the situation since we're [deliberately downloading](https://git
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556353522)
I like the new version, but I want to give the [extract](https://en.cppreference.com/w/cpp/container/map/extract) another try, see if you like it more - if you don't please resolve this, no explanation needed.
With erase+emplace you destroy the node and construct a new one, with extract you detach the existing tree node, mutate the key in the detached node, reinsert the same node. It's not a huge difference, but if there's a dedicated tool for it since C++17, why not use it, it expressed inte
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556353522)
I like the new version, but I want to give the [extract](https://en.cppreference.com/w/cpp/container/map/extract) another try, see if you like it more - if you don't please resolve this, no explanation needed.
With erase+emplace you destroy the node and construct a new one, with extract you detach the existing tree node, mutate the key in the detached node, reinsert the same node. It's not a huge difference, but if there's a dedicated tool for it since C++17, why not use it, it expressed inte
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556829022)
Forgot to ask: what does the `/` mean in `then we consider it stale / for rebroadcasting`?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556829022)
Forgot to ask: what does the `/` mean in `then we consider it stale / for rebroadcasting`?
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556560989)
isn't there an asymmetry between `PrivateBroadcast::Add` where we are skipping adding by `tx->GetHash()` only, but not removing if `by_txid->second.tx->GetWitnessHash() == tx->GetWitnessHash()`?
https://github.com/bitcoin/bitcoin/blob/7826148b12048a40919061d935b1abb2cd49dcb3/src/private_broadcast.cpp#L16-L17
The tests seem to be failing if I check wtxid for insertion as well...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556560989)
isn't there an asymmetry between `PrivateBroadcast::Add` where we are skipping adding by `tx->GetHash()` only, but not removing if `by_txid->second.tx->GetWitnessHash() == tx->GetWitnessHash()`?
https://github.com/bitcoin/bitcoin/blob/7826148b12048a40919061d935b1abb2cd49dcb3/src/private_broadcast.cpp#L16-L17
The tests seem to be failing if I check wtxid for insertion as well...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556667714)
> Hmm, is this a general frowning against size_t?
From my side it is, I don't want to think about architecture for code that's unrelated to it.
> My thinking is that CPUs work best with word sized integers
Making it smaller gives the CPU more information, not less, so it can still revert to storing it aligned on more space (the opposite isn't really possible if we already define it as max size)
> 64 bit integers on 32 bit CPUs will bloat the program unnecessary and 32 bit integers on
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2556667714)
> Hmm, is this a general frowning against size_t?
From my side it is, I don't want to think about architecture for code that's unrelated to it.
> My thinking is that CPUs work best with word sized integers
Making it smaller gives the CPU more information, not less, so it can still revert to storing it aligned on more space (the opposite isn't really possible if we already define it as max size)
> 64 bit integers on 32 bit CPUs will bloat the program unnecessary and 32 bit integers on
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553965873)
nit: seems to be copied from somewhere else:
```suggestion
# Copyright (c) 2023-present The Bitcoin Core developers
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553965873)
nit: seems to be copied from somewhere else:
```suggestion
# Copyright (c) 2023-present The Bitcoin Core developers
```
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557682639)
[78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0)
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557682639)
[78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0)
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557682926)
Sounds good -> [78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0)
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557682926)
Sounds good -> [78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0)
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557683518)
Thanks!
Fixed in [78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0).
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2557683518)
Thanks!
Fixed in [78d6402458..c2088a7458](https://github.com/bitcoin/bitcoin/compare/78d6402458d7d14533444d893c989f0534a3896f..c2088a74586742cadc0041cab8782c423eda3bd0).
💬 enirox001 commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3572670833)
tACK c0bfe72
Reproduced bug on master (test fails with "Invalid characters in payload")
and verified the fix works. All descriptor_tests pass.
The switch to string_view correctly handles string literal null terminators.
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3572670833)
tACK c0bfe72
Reproduced bug on master (test fails with "Invalid characters in payload")
and verified the fix works. All descriptor_tests pass.
The switch to string_view correctly handles string literal null terminators.
🤔 enirox001 reviewed a pull request: "Change Parse descriptor argument to string_view"
(https://github.com/bitcoin/bitcoin/pull/33914#pullrequestreview-3502191427)
tACK c0bfe72
Reproduced bug on master (test fails with "Invalid characters in payload")
and verified the fix works. All descriptor_tests pass.
The switch to string_view correctly handles string literal null terminators.
(https://github.com/bitcoin/bitcoin/pull/33914#pullrequestreview-3502191427)
tACK c0bfe72
Reproduced bug on master (test fails with "Invalid characters in payload")
and verified the fix works. All descriptor_tests pass.
The switch to string_view correctly handles string literal null terminators.
💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3572692780)
rfm?
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3572692780)
rfm?
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3572749774)
Thanks!
> Left minor comment/question in related presentation: https://docs.google.com/presentation/d/1Zez-6DApKRu59kke4i_g9jwxQlaFKKRpOPdYFYsFXfA/edit?disco=AAABwl3JY0U
Good point - I've changed a bit the slide, and added a few comments.
This should be similar to the [electrs schema](https://github.com/romanz/electrs/blob/master/doc/schema.md), but with `txnum` instead of block height.
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3572749774)
Thanks!
> Left minor comment/question in related presentation: https://docs.google.com/presentation/d/1Zez-6DApKRu59kke4i_g9jwxQlaFKKRpOPdYFYsFXfA/edit?disco=AAABwl3JY0U
Good point - I've changed a bit the slide, and added a few comments.
This should be similar to the [electrs schema](https://github.com/romanz/electrs/blob/master/doc/schema.md), but with `txnum` instead of block height.
💬 151henry151 commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3572809238)
The `process_messages` fuzz test fails with `Assertion failed: (mempoolDuplicate.HaveCoin(txin.prevout))` at `txmempool.cpp:727` after allowing NONSTANDARD scripts with ≤15 sigops to pass `AreInputsStandard`.
I added a coin validity check in `AreInputsStandard` (verifying `coin.IsSpent()` before `GetSigOpCount`), but the test still fails. Since this PR only aligns policy behavior and shouldn't require mempool changes, perhaps the issue is how the policy change interacts with the consistency c
...
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3572809238)
The `process_messages` fuzz test fails with `Assertion failed: (mempoolDuplicate.HaveCoin(txin.prevout))` at `txmempool.cpp:727` after allowing NONSTANDARD scripts with ≤15 sigops to pass `AreInputsStandard`.
I added a coin validity check in `AreInputsStandard` (verifying `coin.IsSpent()` before `GetSigOpCount`), but the test still fails. Since this PR only aligns policy behavior and shouldn't require mempool changes, perhaps the issue is how the policy change interacts with the consistency c
...
⚠️ jdavidthomson opened an issue: "Spherical chains of Bitcoin Core"
(https://github.com/bitcoin/bitcoin/issues/33941)
### Please describe the feature you'd like to see added.
Spherical chains
### Is your feature related to a problem, if so please describe it.
Spherical chains
### Describe the solution you'd like
Current btc genesis block is of a known pytz. There are 21,000,000 coins.
This cannot be a new idea:
Are we able to modify the ideas around btc to ensure that there is a progressive addition of chains that have an origin chain with a genesis block of:
0000000040e73bc82487a271757458f091ba9ecf
...
(https://github.com/bitcoin/bitcoin/issues/33941)
### Please describe the feature you'd like to see added.
Spherical chains
### Is your feature related to a problem, if so please describe it.
Spherical chains
### Describe the solution you'd like
Current btc genesis block is of a known pytz. There are 21,000,000 coins.
This cannot be a new idea:
Are we able to modify the ideas around btc to ensure that there is a progressive addition of chains that have an origin chain with a genesis block of:
0000000040e73bc82487a271757458f091ba9ecf
...
💬 monlovesmango commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3573168495)
> I think it would be reasonable to stop this transaction from being created and throw a "invalid parameters" error, telling the user they selected inputs that can't be spent due to policy.
@glozow I created 2 commits that build off of this pr's branch in an attempt to address this here:
https://github.com/monlovesmango/bitcoin/tree/pr33528-add-invalid-parameters-error
Any feedback is greatly appreciated. Feel free to take anything you think is useful. If you don't think its appropriate f
...
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3573168495)
> I think it would be reasonable to stop this transaction from being created and throw a "invalid parameters" error, telling the user they selected inputs that can't be spent due to policy.
@glozow I created 2 commits that build off of this pr's branch in an attempt to address this here:
https://github.com/monlovesmango/bitcoin/tree/pr33528-add-invalid-parameters-error
Any feedback is greatly appreciated. Feel free to take anything you think is useful. If you don't think its appropriate f
...
💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558058740)
Removed the change from `constexpr`.
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558058740)
Removed the change from `constexpr`.