📝 furszy opened a pull request: "index: prevent race by calling 'CustomInit' prior setting 'synced' flag"
(https://github.com/bitcoin/bitcoin/pull/27720)
Decoupled from #27607.
Fixed a potential race condition in master (not possible so far) that could become a actual issue soon.
Where the index's `CustomAppend` method could be called (from `BlockConnected`) before its
`CustomInit` method, causing the index to try to update itself before it is initialized.
This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
before calling to the child class init function (`CustomInit`). So, for example, the
...
(https://github.com/bitcoin/bitcoin/pull/27720)
Decoupled from #27607.
Fixed a potential race condition in master (not possible so far) that could become a actual issue soon.
Where the index's `CustomAppend` method could be called (from `BlockConnected`) before its
`CustomInit` method, causing the index to try to update itself before it is initialized.
This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
before calling to the child class init function (`CustomInit`). So, for example, the
...
💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557536125)
> > Tripling the outgoing bandwidth load for peers selected for hb compact block relay seems concerning; not sure if I'd call it a blocker though.
>
> To be clear, this is only a tripling in the case where where all your peers are roughly equally as fast, and triple of _what_ is the most important thing.
In an "everyone's honest" environment, I think the most likely reason for there to be enough of a delay between `cmpctblock / getblocktxns / blocktxns` that there'll be another `cmpctblock
...
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557536125)
> > Tripling the outgoing bandwidth load for peers selected for hb compact block relay seems concerning; not sure if I'd call it a blocker though.
>
> To be clear, this is only a tripling in the case where where all your peers are roughly equally as fast, and triple of _what_ is the most important thing.
In an "everyone's honest" environment, I think the most likely reason for there to be enough of a delay between `cmpctblock / getblocktxns / blocktxns` that there'll be another `cmpctblock
...
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1557553941)
Feedback tackled, thanks ryanofsky and mzumzande for your deep review!
This PR now depends on #27720 and #27708.
List of changes since the last review:
* Created #27708 so the post-init shutdown doesn’t introduce any tangible difference for user and tests. (Per https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1198184049 discussion).
* Moved the `LoadMempool()` call from the blockstorage import function to init.cpp (per https://github.com/bitcoin/bitcoin/pull/27607#discussion
...
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1557553941)
Feedback tackled, thanks ryanofsky and mzumzande for your deep review!
This PR now depends on #27720 and #27708.
List of changes since the last review:
* Created #27708 so the post-init shutdown doesn’t introduce any tangible difference for user and tests. (Per https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1198184049 discussion).
* Moved the `LoadMempool()` call from the blockstorage import function to init.cpp (per https://github.com/bitcoin/bitcoin/pull/27607#discussion
...
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200766163)
For serializing the strings for the prefix, the stream type and client version don't matter so I just didn't use them.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200766163)
For serializing the strings for the prefix, the stream type and client version don't matter so I just didn't use them.
💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557558059)
> > I know I sound like a broken record here but... is that a guarantee? If so I think it should be documented.
>
> I think possibly I'm not understanding the question. It seems self-evident that if some kernel function calls the `kernel::Notifications::fatalError` method, that function won't finish executing until the `kernel::Notifications::fatalError` method implementation completes and returns. Function calls in c++ are blocking, so functions don't return until the functions they call ret
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1557558059)
> > I know I sound like a broken record here but... is that a guarantee? If so I think it should be documented.
>
> I think possibly I'm not understanding the question. It seems self-evident that if some kernel function calls the `kernel::Notifications::fatalError` method, that function won't finish executing until the `kernel::Notifications::fatalError` method implementation completes and returns. Function calls in c++ are blocking, so functions don't return until the functions they call ret
...
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777487)
I think it's fine to leave it as is.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777487)
I think it's fine to leave it as is.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777808)
Adopted these suggestions.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200777808)
Adopted these suggestions.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200778103)
Added the case and squashed.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200778103)
Added the case and squashed.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200778268)
Removed
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200778268)
Removed
💬 Bandileds commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1557572227)
I love it
(https://github.com/bitcoin/bitcoin/pull/25796#issuecomment-1557572227)
I love it
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200779393)
Added a line in the commit message.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200779393)
Added a line in the commit message.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200783492)
I've changed it to increment unconditionally as mentioned below. Ultimately, `wss` gets removed so I don't think the correctness of this counting particularly matters.
Additionally, the current behavior doesn't use the counts when there are failures, so having it be inaccurate in such cases shouldn't matter.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200783492)
I've changed it to increment unconditionally as mentioned below. Ultimately, `wss` gets removed so I don't think the correctness of this counting particularly matters.
Additionally, the current behavior doesn't use the counts when there are failures, so having it be inaccurate in such cases shouldn't matter.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200784032)
Changed to increment unconditionally.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200784032)
Changed to increment unconditionally.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200784391)
Done
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200784391)
Done
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200784861)
Added a comment.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200784861)
Added a comment.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200786113)
Changed to increment unconditionally, but these counters all get dropped in a later commit anyways.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200786113)
Changed to increment unconditionally, but these counters all get dropped in a later commit anyways.
📝 theuni opened a pull request: "depends: remove redundant stdlib option"
(https://github.com/bitcoin/bitcoin/pull/27721)
Like #27628, this is another dependency of #21778, though it doesn't become obvious until used with a newer clang.
This should be a no-op.
Use of -stdlib++-isystem gets rid of any system c++ header include paths and negates the need for this option. In newer versions of clangs the combo produces an annoying warning that actually causes problems during configure.
(https://github.com/bitcoin/bitcoin/pull/27721)
Like #27628, this is another dependency of #21778, though it doesn't become obvious until used with a newer clang.
This should be a no-op.
Use of -stdlib++-isystem gets rid of any system c++ header include paths and negates the need for this option. In newer versions of clangs the combo produces an annoying warning that actually causes problems during configure.
💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1557593304)
> So I guess we'll need to patch it out of guix.
Suggestions for how to go about this are welcome. It's not at all straightforward to me how to patch out.
The llvm toolchain bumps are currently blocked on this :(
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1557593304)
> So I guess we'll need to patch it out of guix.
Suggestions for how to go about this are welcome. It's not at all straightforward to me how to patch out.
The llvm toolchain bumps are currently blocked on this :(
🤔 stickies-v reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1436822917)
Code review ACK 96b513f605fb2df441b66da583056b1c8acd4dbc, but I think the first commit should be removed from this PR, I think it's an accident?
I think the code is good and suits the intent of the PR well. My only concern is about the potential footgun introduced, even if it is relatively mild. Anyone running bitcoind with `whitebind=0.0.0.0:<port>` will now be vulnerable to having all their inbound non-whitelisted slots taken over by an attacker that has figured out what the whitelisted por
...
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1436822917)
Code review ACK 96b513f605fb2df441b66da583056b1c8acd4dbc, but I think the first commit should be removed from this PR, I think it's an accident?
I think the code is good and suits the intent of the PR well. My only concern is about the potential footgun introduced, even if it is relatively mild. Anyone running bitcoind with `whitebind=0.0.0.0:<port>` will now be vulnerable to having all their inbound non-whitelisted slots taken over by an attacker that has figured out what the whitelisted por
...
💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200632788)
nit: if `vEvictionCandidates` is empty, there's no point (I think, couldn't see any side effects in `EraseLastKElements` or `ProtectEvictionCandidatesByRatio`) executing the next lines so might as well return early? Unless you think this makes the code less clear?
```suggestion
if (vEvictionCandidates.empty()) return std::nullopt;
std::optional<NodeId> force_evict;
if (force) {
```
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200632788)
nit: if `vEvictionCandidates` is empty, there's no point (I think, couldn't see any side effects in `EraseLastKElements` or `ProtectEvictionCandidatesByRatio`) executing the next lines so might as well return early? Unless you think this makes the code less clear?
```suggestion
if (vEvictionCandidates.empty()) return std::nullopt;
std::optional<NodeId> force_evict;
if (force) {
```