💬 sipa commented on pull request "Introduce field element and group element classes to test_framework/key.py":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1192016465)
Nice catch, fixed.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1192016465)
Nice catch, fixed.
💬 sipa commented on pull request "Introduce field element and group element classes to test_framework/key.py":
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1545314256)
@theStack I've included your commit to add the precomputed G table. The speedup is significant enough that it's worth it, I think.
You've indeed discovered one of the techniques that are used for speeding up EC multiplications with precomputed tabled. Libsecp256k1 today uses a more advanced version of that idea, where all multiples of the form `i*16^j*G` for all i=0..15, and j=0..63 are precomputed, leaving us with ~63 point additions. An even more advanced approach is discussed in https://gi
...
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1545314256)
@theStack I've included your commit to add the precomputed G table. The speedup is significant enough that it's worth it, I think.
You've indeed discovered one of the techniques that are used for speeding up EC multiplications with precomputed tabled. Libsecp256k1 today uses a more advanced version of that idea, where all multiples of the form `i*16^j*G` for all i=0..15, and j=0..63 are precomputed, leaving us with ~63 point additions. An even more advanced approach is discussed in https://gi
...
⚠️ MarcoFalke opened an issue: "--with-sanitizers=float-divide-by-zero crash with -debug=bench in Chainstate::ConnectTip"
(https://github.com/bitcoin/bitcoin/issues/27635)
Not sure if this UB causes issues with any compiler. clang is fine, see:
* `-fsanitize=float-divide-by-zero`: Floating point division by zero. This is undefined per the C and C++ standards, but is defined by Clang (and by ISO/IEC/IEEE 60559 / IEEE 754) as producing either an infinity or NaN value, so is not included in `-fsanitize=undefined`.
To reproduce just compile `--with-sanitizers=float-divide-by-zero` and run something, for example `bitcoind`:
```
UBSAN_OPTIONS="suppressions=$
...
(https://github.com/bitcoin/bitcoin/issues/27635)
Not sure if this UB causes issues with any compiler. clang is fine, see:
* `-fsanitize=float-divide-by-zero`: Floating point division by zero. This is undefined per the C and C++ standards, but is defined by Clang (and by ISO/IEC/IEEE 60559 / IEEE 754) as producing either an infinity or NaN value, so is not included in `-fsanitize=undefined`.
To reproduce just compile `--with-sanitizers=float-divide-by-zero` and run something, for example `bitcoind`:
```
UBSAN_OPTIONS="suppressions=$
...
📝 TheCharlatan opened a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this
...
(https://github.com/bitcoin/bitcoin/pull/27636)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this
...
💬 MarcoFalke commented on pull request "ci: Remove CI_EXEC bloat":
(https://github.com/bitcoin/bitcoin/pull/27616#discussion_r1192054525)
Moving the `exit 0` may also fix a bug where the CI Pod for ` ARM64 Android APK [jammy] ` is left alive even if the test passed.
(https://github.com/bitcoin/bitcoin/pull/27616#discussion_r1192054525)
Moving the `exit 0` may also fix a bug where the CI Pod for ` ARM64 Android APK [jammy] ` is left alive even if the test passed.
💬 Sjors commented on pull request "[23.2] Backports for rc1":
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1545359195)
I didn't check the #27608 backport (nor the original). I did check #27608, the other commits in this PR and the release notes, which look good to me.
(https://github.com/bitcoin/bitcoin/pull/27624#issuecomment-1545359195)
I didn't check the #27608 backport (nor the original). I did check #27608, the other commits in this PR and the release notes, which look good to me.
💬 vasild commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1545379692)
No, wait! This PR changes both Case1 and Case2, but Case1 is only about things happening in the same second.
What I tried yesterday:
* Run the newly added tests, they were passing, of course. Cool!
* Revert the `net_processing.cpp` changes, both tests `Request the irrelevant transaction even though it has not being offered to us` and `Create a third transaction (that was therefore not part of the mempool message) and try to...` failed. Good! To reach the second test I disabled the `assert`
...
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1545379692)
No, wait! This PR changes both Case1 and Case2, but Case1 is only about things happening in the same second.
What I tried yesterday:
* Run the newly added tests, they were passing, of course. Cool!
* Revert the `net_processing.cpp` changes, both tests `Request the irrelevant transaction even though it has not being offered to us` and `Create a third transaction (that was therefore not part of the mempool message) and try to...` failed. Good! To reach the second test I disabled the `assert`
...
💬 fanquake commented on pull request "Introduce field element and group element classes to test_framework/key.py":
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1545399444)
cc @real-or-random
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1545399444)
cc @real-or-random
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1545399521)
reACK af86462
Now using const reference in `parse()`.
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1545399521)
reACK af86462
Now using const reference in `parse()`.
💬 vasild commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1545400201)
Looking at the comment of `CRollingBloomFilter` if `m_recently_announced_invs` is changed from `3500` to `300000` then its size would increase from 37KB to 3MB. I guess that is too much and maybe was the reason why `MEMPOOL` replies don't use `m_recently_announced_invs` and instead use the `m_last_mempool_req` timing mechanism.
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1545400201)
Looking at the comment of `CRollingBloomFilter` if `m_recently_announced_invs` is changed from `3500` to `300000` then its size would increase from 37KB to 3MB. I guess that is too much and maybe was the reason why `MEMPOOL` replies don't use `m_recently_announced_invs` and instead use the `m_last_mempool_req` timing mechanism.
💬 fanquake commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#issuecomment-1545400377)
cc @stratospher
(https://github.com/bitcoin/bitcoin/pull/27479#issuecomment-1545400377)
cc @stratospher
📝 Riahiamirreza opened a pull request: "rpc: show P2(W)SH redeemScript in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/27637)
This a work in progress PR. I'll squash all my commits at the end to make commits atomic (as mentioned [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches))
It's an effort to solve this issue https://github.com/bitcoin/bitcoin/issues/27391.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https
...
(https://github.com/bitcoin/bitcoin/pull/27637)
This a work in progress PR. I'll squash all my commits at the end to make commits atomic (as mentioned [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches))
It's an effort to solve this issue https://github.com/bitcoin/bitcoin/issues/27391.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https
...
💬 Sjors commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1192111163)
For a followup it would be could to clarify "should be considered" _for what_. It's somewhat confusing to reason about because the call site reverses a and b.
The first case in the comment is "a is not in the mempool, but b is", but the line of code checks if b is not in the mempool.
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1192111163)
For a followup it would be could to clarify "should be considered" _for what_. It's somewhat confusing to reason about because the call site reverses a and b.
The first case in the comment is "a is not in the mempool, but b is", but the line of code checks if b is not in the mempool.
✅ Riahiamirreza closed a pull request: "rpc: show P2(W)SH redeemScript in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/27637)
(https://github.com/bitcoin/bitcoin/pull/27637)
📝 Riahiamirreza opened a pull request: "rpc: show P2(W)SH redeemScript in getrawtransaction #27637"
(https://github.com/bitcoin/bitcoin/pull/27638)
This a work in progress PR. I'll squash all my commits at the end to make commits atomic (as mentioned [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches))
It's an effort to solve this issue https://github.com/bitcoin/bitcoin/issues/27391.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
h
...
(https://github.com/bitcoin/bitcoin/pull/27638)
This a work in progress PR. I'll squash all my commits at the end to make commits atomic (as mentioned [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches))
It's an effort to solve this issue https://github.com/bitcoin/bitcoin/issues/27391.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
h
...
🚀 fanquake merged a pull request: "[23.2] Backports for rc1"
(https://github.com/bitcoin/bitcoin/pull/27624)
(https://github.com/bitcoin/bitcoin/pull/27624)
📝 fanquake locked a pull request: "rpc: show P2(W)SH redeemScript in getrawtransaction"
(https://github.com/bitcoin/bitcoin/pull/27637)
This a work in progress PR. I'll squash all my commits at the end to make commits atomic (as mentioned [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches))
It's an effort to solve this issue https://github.com/bitcoin/bitcoin/issues/27391.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https
...
(https://github.com/bitcoin/bitcoin/pull/27637)
This a work in progress PR. I'll squash all my commits at the end to make commits atomic (as mentioned [here](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches))
It's an effort to solve this issue https://github.com/bitcoin/bitcoin/issues/27391.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https
...
💬 MarcoFalke commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1192123432)
> The first case in the comment is "a is not in the mempool, but b is", but the line of code checks if b is not in the mempool.
If b is not in the mempool, the function must return false, see https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191235815
If b is in the mempool, the function will continue over the early return `false`, and checks to see if a is missing, in which case it should be considered sooner.
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1192123432)
> The first case in the comment is "a is not in the mempool, but b is", but the line of code checks if b is not in the mempool.
If b is not in the mempool, the function must return false, see https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1191235815
If b is in the mempool, the function will continue over the early return `false`, and checks to see if a is missing, in which case it should be considered sooner.
💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1545439229)
>With the "tree only" topology requirement I feel more comfortable having this branch exist, but still don't think this is something to merge yet / backport.
Do you think this is not ready to merge / backport yet because of the unexpected edge cases that showed up with the previous, less restrictive iteration, leading to the expectation that something might show up for "tree only" too? Or is there already a known issue for "tree only"?
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1545439229)
>With the "tree only" topology requirement I feel more comfortable having this branch exist, but still don't think this is something to merge yet / backport.
Do you think this is not ready to merge / backport yet because of the unexpected edge cases that showed up with the previous, less restrictive iteration, leading to the expectation that something might show up for "tree only" too? Or is there already a known issue for "tree only"?
💬 dergoegge commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1192138027)
This could be a `else if` of the outer if statement. That would avoid the nesting here.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1192138027)
This could be a `else if` of the outer if statement. That would avoid the nesting here.