💬 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`.
💬 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_r2558059839)
I actually slightly prefer to keep it as is, because if I do this earlier I also have to change the tests because the hash result changes too with the span usage. I think it makes more sense to have this in the later commit where there is a much bigger overall benefit of introducing span evident that is actually what justifies the hash result being changed.
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558059839)
I actually slightly prefer to keep it as is, because if I do this earlier I also have to change the tests because the hash result changes too with the span usage. I think it makes more sense to have this in the later commit where there is a much bigger overall benefit of introducing span evident that is actually what justifies the hash result being changed.
💬 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_r2558060398)
Renamed to `CheckStandardAsmap` and using it in a two more places, but keeping it out of asmap_direct fuzzer because we are testing the bits there explicitly. Moved it below below the sanity check in .h which is the same location as .cpp
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558060398)
Renamed to `CheckStandardAsmap` and using it in a two more places, but keeping it out of asmap_direct fuzzer because we are testing the bits there explicitly. Moved it below below the sanity check in .h which is the same location as .cpp
💬 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_r2558060593)
Done
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558060593)
Done
💬 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_r2558060750)
Done
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558060750)
Done