💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1715690103)
In 4a955955ec587120fb121e5068e3bc1f80406ac5 "net_processing: handle ConnectionType::PRIVATE_BROADCAST connections": It seems `FinishBroadcast` returns a `bool`, but this value is never used.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1715690103)
In 4a955955ec587120fb121e5068e3bc1f80406ac5 "net_processing: handle ConnectionType::PRIVATE_BROADCAST connections": It seems `FinishBroadcast` returns a `bool`, but this value is never used.
🤔 theStack reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2236038602)
Concept ACK
Lightly reviewed about half of the refactoring commits, left some non-critical stuff and comments on the way below.
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2236038602)
Concept ACK
Lightly reviewed about half of the refactoring commits, left some non-critical stuff and comments on the way below.
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715592140)
```suggestion
// If the chain tip has changed, previously rejected transactions might now be valid, e.g. due
```
also, this comment now exists both in `PeerManagerImpl::ActiveTipChange` and `TxDownloadImpl::ActiveTipChange`, probably one can be removed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715592140)
```suggestion
// If the chain tip has changed, previously rejected transactions might now be valid, e.g. due
```
also, this comment now exists both in `PeerManagerImpl::ActiveTipChange` and `TxDownloadImpl::ActiveTipChange`, probably one can be removed
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715622227)
nit: strictly speaking this is not a pure refactor, as there is a change in logging behaviour (INV messages received during IBD now don't trigger the "got inv" log message any more). seems to make more sense though anyway, so feel free to ignore
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715622227)
nit: strictly speaking this is not a pure refactor, as there is a change in logging behaviour (INV messages received during IBD now don't trigger the "got inv" log message any more). seems to make more sense though anyway, so feel free to ignore
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715630850)
small note: with this approach, `AlreadyHaveTx` is now called twice: once in the if condition, and the second time in the `AddTxAnnouncement` method (not sure if that's really a problem though)
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715630850)
small note: with this approach, `AlreadyHaveTx` is now called twice: once in the if condition, and the second time in the `AddTxAnnouncement` method (not sure if that's really a problem though)
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715600184)
slightly-more-modern-nit (here and in `TxDownloadImpl::DisconnectedPeer` below):
```suggestion
if (m_peer_info.contains(nodeid)) return;
```
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715600184)
slightly-more-modern-nit (here and in `TxDownloadImpl::DisconnectedPeer` below):
```suggestion
if (m_peer_info.contains(nodeid)) return;
```
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2286816238)
I rebased https://github.com/Sjors/bitcoin/pull/48 to take this PR and https://github.com/bitcoin/bitcoin/pull/30509 into account. Already looked at few commits, but got distracted by a bug on my end, so will continue later.
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2286816238)
I rebased https://github.com/Sjors/bitcoin/pull/48 to take this PR and https://github.com/bitcoin/bitcoin/pull/30509 into account. Already looked at few commits, but got distracted by a bug on my end, so will continue later.
🤔 mzumsande reviewed a pull request: "validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2236239890)
tested ACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410
I re-created the snapshot at height 840k and verified that the file identical to the one I downloaded from the torrent.
I also successfully loaded the snapshot into a new node (though I didn't complete the sync).
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2236239890)
tested ACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410
I re-created the snapshot at height 840k and verified that the file identical to the one I downloaded from the torrent.
I also successfully loaded the snapshot into a new node (though I didn't complete the sync).
💬 TheBlueMatt commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286856946)
> Update the BIP first?
The BIP was originally 600, and then was updated to follow the code, so I figured I'd do the same :)
> I'm confused why is this touching ancient stuff related to every net?
Cause I was distracted and just blindly tracing constants back without thinking :sweat_smile:
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286856946)
> Update the BIP first?
The BIP was originally 600, and then was updated to follow the code, so I figured I'd do the same :)
> I'm confused why is this touching ancient stuff related to every net?
Cause I was distracted and just blindly tracing constants back without thinking :sweat_smile:
💬 vasild commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286883042)
Maybe `0b00000000'00000000'00000000'00001000` is a bit less horrific ;) The current is ok, no need to change it further.
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286883042)
Maybe `0b00000000'00000000'00000000'00001000` is a bit less horrific ;) The current is ok, no need to change it further.
📝 justinvforvendetta opened a pull request: "remove repetitive wording"
(https://github.com/bitcoin/bitcoin/pull/30649)
fixes some grammar
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
(https://github.com/bitcoin/bitcoin/pull/30649)
fixes some grammar
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2286925458)
Fixed noise encryption regression, see https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2286722016
The current commit is tagged [sv2-tp-0.1.7](https://github.com/Sjors/bitcoin/releases/tag/sv2-tp-0.1.7), binaries coming soon.
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2286925458)
Fixed noise encryption regression, see https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2286722016
The current commit is tagged [sv2-tp-0.1.7](https://github.com/Sjors/bitcoin/releases/tag/sv2-tp-0.1.7), binaries coming soon.
✅ justinvforvendetta closed a pull request: "remove repetitive wording"
(https://github.com/bitcoin/bitcoin/pull/30649)
(https://github.com/bitcoin/bitcoin/pull/30649)
📝 justinvforvendetta opened a pull request: "removes repeated words, fix grammar"
(https://github.com/bitcoin/bitcoin/pull/30650)
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30650)
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 mzumsande commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2286970765)
This is still really confusing to me.
I don't understand tracepoints in depth, but it looks like there is a race between bicoind continuing to run (and overwriting data after it goes out of scope), and the data-collecting program (bpftrace) reading data from the memory location the `c_str()` result points to. But if that was the case, it would be a very general problem with tracepoints, and I don't see how it would specifically lead to failures in this exact situation with the `mempool-rejec
...
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2286970765)
This is still really confusing to me.
I don't understand tracepoints in depth, but it looks like there is a race between bicoind continuing to run (and overwriting data after it goes out of scope), and the data-collecting program (bpftrace) reading data from the memory location the `c_str()` result points to. But if that was the case, it would be a very general problem with tracepoints, and I don't see how it would specifically lead to failures in this exact situation with the `mempool-rejec
...
✅ justinvforvendetta closed a pull request: "removes repeated words, fix grammar"
(https://github.com/bitcoin/bitcoin/pull/30650)
(https://github.com/bitcoin/bitcoin/pull/30650)
📝 fanquake locked a pull request: "remove repetitive wording"
(https://github.com/bitcoin/bitcoin/pull/30649)
fixes some grammar
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
(https://github.com/bitcoin/bitcoin/pull/30649)
fixes some grammar
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
🤔 furszy reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2236408618)
ACK 0240454120a88915396b0eb68d9b5bee73e52fed
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2236408618)
ACK 0240454120a88915396b0eb68d9b5bee73e52fed
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2286994610)
> I guess another thing I'd like to know is if this is the initial C API, and the implementation is around 3000 lines, and it doesn't handle "transactions, block headers, coins cache, utxo set, meta data, and the mempool", how much bigger do you think it will get if it does cover most of the things you would like it to cover? Like is this 20%, 30%, or 50% of the expected size?
I think a fair comparison would be comparing the amount of code "glue" required, e.g. the size of the `bitcoinkernel.
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2286994610)
> I guess another thing I'd like to know is if this is the initial C API, and the implementation is around 3000 lines, and it doesn't handle "transactions, block headers, coins cache, utxo set, meta data, and the mempool", how much bigger do you think it will get if it does cover most of the things you would like it to cover? Like is this 20%, 30%, or 50% of the expected size?
I think a fair comparison would be comparing the amount of code "glue" required, e.g. the size of the `bitcoinkernel.
...
📝 justinvforvendetta opened a pull request: "remove repeated word in note"
(https://github.com/bitcoin/bitcoin/pull/30651)
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30651)
<!--
*** 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://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...