Bitcoin Core Github
44 subscribers
121K links
Download Telegram
🤔 hebasto reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1425576361)
Approach ACK 97b0e28d6f5e86e8c7ee0775d22cd923dabca09d.

In d84952cd64bd03ce444bbf6077d060c92526d040 commit message:
s/"to FatalError"/"to FatalErrorf"/

Maybe make it a scripted-diff?
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1546945193)
nit: https://api.cirrus-ci.com/v1/task/4542005133443072/logs/ci.log:
```
node/validation_notifications.h should add these lines:
enum class SynchronizationState;
```

```
node/validation_notifications.cpp should add these lines:
enum class SynchronizationState;

node/validation_notifications.cpp should remove these lines:
- #include <kernel/validation_notifications_interface.h>
```
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1546961637)
Thank you for the review @hebasto,

Updated 97b0e28d6f5e86e8c7ee0775d22cd923dabca09d -> 2f9c2d245360b3fad6d469a76c2916d75b027b57 ([chainstateRmKernelUiInterface_2](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_2) -> [chainstateRmKernelUiInterface_3](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_2..chainstateRmKernelUiInterface_3)).

* Add
...
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1193210027)
I've just added a test for this on the libsecp256k1 side: https://github.com/bitcoin-core/secp256k1/pull/1129/files#diff-d7678006ab19bb19a4a8dfb5f497cab0127e33730791c6e0597db203751322d9R365

I may do something similar here when I update again.
💬 ajtowns commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/27630#issuecomment-1547072913)
> In the presence of smaller transactions on the network, blocks can sustain a higher relay rate than 7tx/second.

I agree this limit is too small after segwit. I think plausible datapoints for the rate at which we can sustain confirmable txs across the network might be:

* pre-segwit, 2-in, 2-out p2pkh: 374 vb, ~4.5 tx/s
* 2-in, 2-out p2wpkh: 208.5 vb, ~8 tx/s

* pre-segwit, 1-in, 1-out p2pkh: 192 vb, ~8.5 tx/s
* 1-in, 1-out p2wpkh: 109.5 vb, ~15 tx/s
* 1-in p2tr, 1-out p2wpkh:
...
💬 MarcoFalke 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#discussion_r1193429237)
I was trying to say that if any false negative or false positive arises as a result of a mempool message in this pull request, the same should already be observable (on master or releases) in the absence of a mempool message with normal tx relay? So addressing it here seems unrelated? I presume #27630 is addressing it.
💬 vasild commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1547398410)
One more thing to consider - this PR is a followup to another one (not an out of the blue, standalone one). We use the will-do-in-a-followup practice to avoid invalidating ACKs on the original PR with non-critical changes. If such followup PRs start getting frowned upon or ignored, then that could create a backward pressure to insist to include the small/non-critical suggestions in the original PR because the followup PR will not make it.

@stratospher, @mzumsande, maybe you want to review thi
...
📝 MarcoFalke opened a pull request: "doc: Remove unused NO_BLOOM_VERSION constant"
(https://github.com/bitcoin/bitcoin/pull/27657)
This source code is the wrong place to document historic and now irrelevant details. Also, while touching the docs, clarify that the BIP 35 `mempool` message type is currently also guarded by the BIP 111 `NODE_BLOOM` flag, even though BIP 111 does not mention the `mempool` message type.
💬 MarcoFalke commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#discussion_r1193500972)
#27657
👍 vasild approved a pull request: "Raise on invalid -debug and -loglevel config options"
(https://github.com/bitcoin/bitcoin/pull/27632#pullrequestreview-1426041286)
ACK 9c74c94714465ac66d82a7e233312d188c9b2841
📝 hebasto opened a pull request: "test: Drop `deadlock:libdb` TSan suppression"
(https://github.com/bitcoin/bitcoin/pull/27658)
I'm not able to reproduce the TSan's deadlock warning for `libdb` for `24.x`, `25.x` and master branches neither in CI nor in different local environments (Ubuntu, Fedora).

If Tsan complains again, we can obtain a call stack from TSan, and this change can be easily reverted back.

Noticed while reviewing https://github.com/bitcoin/bitcoin/pull/27556.
🤔 vasild reviewed a pull request: "p2p: Increase tx relay rate"
(https://github.com/bitcoin/bitcoin/pull/27630#pullrequestreview-1426068855)
Concept ACK on the increase.

I don't feel confident enough to judge the exact numbers proposed.
💬 0xB10C commented on pull request "doc: Remove unused NO_BLOOM_VERSION constant":
(https://github.com/bitcoin/bitcoin/pull/27657#issuecomment-1547444768)
ACK facbcd3
💬 ajtowns 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#discussion_r1193531102)
The scenario here is:

* issue MEMPOOL with a large , returning 6000 txs from the mempool, all of which are added to the bloom filter, which then causes the first ~750 or more to be removed from the rolling bloom filter as it rolls over
* request one of those txs via GETDATA, which then fails if the tx hasn't been in the mempool for more than 2 minutes

In normal relay, over the course of two minutes we'd only advertise ~840 txs (~2100 for an outbound), and only add the txs to the bloom
...
💬 MarcoFalke 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#discussion_r1193547718)
Whoops. I was incorrectly assuming that a reply to a `mempool` message was rate-limited and size-limited. (Which on a second thought, probably doesn't make sense to do). So a single reply can hold up to 50'000 inv entries, which will certainly overflow. Sorry for the distraction.
💬 kroese commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1547489470)
I have run into an issue where I cannot verify the signatures for test releases (like v24rc3 or v25rc2). When I execute:

`gpg --verify SHA256SUMS.asc SHA256SUMS`

It returns exitcode 2, while on the final releases it works fine.

What could be causing this behaviour? Or is it to be expected that test releases are not signed?
⚠️ GregTonoski opened an issue: ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives"
(https://github.com/bitcoin/bitcoin/issues/27659)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

"The amount exceeds you balance." error message after clicking "Create Unsigned" button even though there are funds on an address.

![screenshot](https://github.com/bitcoin/bitcoin/assets/111286121/60f8ab66-e44d-4c99-9cd4-4fdd05c72595)


### Expected behaviour

A message about steps to workaround or solve the issue, e.g. "Balance must be above 0 in order to create PSBT. Do you want to s
...
💬 MarcoFalke 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#discussion_r1193582239)
Side note: The 50k max inv size also seems to overflow, which is probably another source for false negatives, depending on the use case.

<img src=https://user-images.githubusercontent.com/19157360/235448790-6a1448bd-64b3-4c41-89d3-1ca5bf0cf76d.png></img>

Image stolen from the comment https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174 by 0xB10C .
📝 fanquake opened a pull request: "[24.1] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27660)
Final changes for `v24.1`.
PR for bitcoincore.org is here: https://github.com/bitcoin-core/bitcoincore.org/pull/968.
👍 hebasto approved a pull request: "[24.1] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27660#pullrequestreview-1426164333)
ACK 89a5a416deac060fed8c21d4381c8f59da4f4187, I have reviewed the code and it looks OK.