💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085359836)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077503435
> nit: Could use util/string.h:
That's slightly simpler, but doesn't seem enough to want to construct and unneeded vector and pull in an extra dependency here. Worth keeping in mind if this code needs to be extended though.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085359836)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077503435
> nit: Could use util/string.h:
That's slightly simpler, but doesn't seem enough to want to construct and unneeded vector and pull in an extra dependency here. Worth keeping in mind if this code needs to be extended though.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085292210)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942
Thanks, fixed both
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085292210)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942
Thanks, fixed both
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085306496)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474
Agree it would be nice to add a friendlier error message that would suggest how to fix the problem. Added this to the list of followups in the PR description that could make the command nicer & easier to use. My goal is to start off with the simplest working implementation and more niceties from there.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085306496)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474
Agree it would be nice to add a friendlier error message that would suggest how to fix the problem. Added this to the list of followups in the PR description that could make the command nicer & easier to use. My goal is to start off with the simplest working implementation and more niceties from there.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085373623)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077548018
> nit: Could verify we succeeded:
Good point. An assert seems extreme here since there could be some OS variation, but this case wasn't being handled well in the calling function and could do things like try to execute the same path twice. Add better handling now in caller.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085373623)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077548018
> nit: Could verify we succeeded:
Good point. An assert seems extreme here since there could be some OS variation, but this case wasn't being handled well in the calling function and could do things like try to execute the same path twice. Add better handling now in caller.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085361665)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077515690
> nit: Could keep style consistent with function body rather than wrapped function.
Thanks, I can never remember where the spaces go.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085361665)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077515690
> nit: Could keep style consistent with function body rather than wrapped function.
Thanks, I can never remember where the spaces go.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085351896)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077299356
Good catch. If the vector was reallocated and string objects were using a small string optimization, the previous code could easily result in passing pointers to already freed memory. Fixed by adding a separate loop to construct new_argv. Also removed the c cast as that was unnecessary.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085351896)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077299356
Good catch. If the vector was reallocated and string objects were using a small string optimization, the previous code could easily result in passing pointers to already freed memory. Fixed by adding a separate loop to construct new_argv. Also removed the c cast as that was unnecessary.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085362931)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077517153
Good catch, removed
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085362931)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077517153
Good catch, removed
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085288505)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2048440135
> if you retouch
Thanks, updated
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085288505)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2048440135
> if you retouch
Thanks, updated
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085384043)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405
> nit: Child executable names leak through
Good suggestion, added it to the list in the PR description.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085384043)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405
> nit: Child executable names leak through
Good suggestion, added it to the list in the PR description.
🤔 pablomartin4btc reviewed a pull request: "test: Remove legacy wallet RPC overloads"
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2834574797)
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
Commit reorg proposed by @Sjors made sense.
On first commit, [test: Remove unnecessary importprivkey from wallet_createwallet](https://github.com/bitcoin/bitcoin/pull/32452/commits/94c87bbbd06eb9a57930b9f59315533cfbe8f460), the reasoning is more that `importprivkey` was only compatible with legacy wallets.
On the second commit, [test: Replace importprivkey with wallet_importprivkey](https://github.com/bitcoin/bitcoin/pull/32452/commits/fcc
...
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2834574797)
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
Commit reorg proposed by @Sjors made sense.
On first commit, [test: Remove unnecessary importprivkey from wallet_createwallet](https://github.com/bitcoin/bitcoin/pull/32452/commits/94c87bbbd06eb9a57930b9f59315533cfbe8f460), the reasoning is more that `importprivkey` was only compatible with legacy wallets.
On the second commit, [test: Replace importprivkey with wallet_importprivkey](https://github.com/bitcoin/bitcoin/pull/32452/commits/fcc
...
🤔 sipa reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2834591758)
> Unrelated to this PR and more of an FYI, in src/test/fuzz/txgraph.cpp pick_fn I think that:
Sure, that would be correct, but very confusing, I think!
The number of possible choices is `tx_count[0] + sims[0].removed.size() + 1`, and this value is stored in the variable `choices`, representing the number of choices.
A number is then generated in the range between `0` and `choices - 1`, inclusive.
Of course, operationally, the `+ 1` and subsequence `- 1` are superfluous, but then the
...
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2834591758)
> Unrelated to this PR and more of an FYI, in src/test/fuzz/txgraph.cpp pick_fn I think that:
Sure, that would be correct, but very confusing, I think!
The number of possible choices is `tx_count[0] + sims[0].removed.size() + 1`, and this value is stored in the variable `choices`, representing the number of choices.
A number is then generated in the range between `0` and `choices - 1`, inclusive.
Of course, operationally, the `+ 1` and subsequence `- 1` are superfluous, but then the
...
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085515956)
> Perhaps mention that GetCurrentChunk should only be called once (which is unusual for a getter), even though I can't think of an scenario where you would actually want to call this more than once.
Why? It's perfectly allowed to call it more than once (though, kind of pointless).
> However I think the best thing to do is actually not use m_cur_cluster for dual purposes and instead have a dedicated variable that tracks when singletons and last chunks don't need to be added to m_excluded_cl
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085515956)
> Perhaps mention that GetCurrentChunk should only be called once (which is unusual for a getter), even though I can't think of an scenario where you would actually want to call this more than once.
Why? It's perfectly allowed to call it more than once (though, kind of pointless).
> However I think the best thing to do is actually not use m_cur_cluster for dual purposes and instead have a dedicated variable that tracks when singletons and last chunks don't need to be added to m_excluded_cl
...
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085526718)
Remember that an `Assume()` is **never** necessary. Its only purpose is helping you, the reviewer, gain confidence that a property is likely true.
As for what it isn't here, I didn't think it'd be useful, as `ClearChunkData` is a very low-level internal function. I put the `Assume()` calls around maintenance largely in higher-level functions, but if you feel it would help you to have more, feel free to suggest them.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085526718)
Remember that an `Assume()` is **never** necessary. Its only purpose is helping you, the reviewer, gain confidence that a property is likely true.
As for what it isn't here, I didn't think it'd be useful, as `ClearChunkData` is a very low-level internal function. I put the `Assume()` calls around maintenance largely in higher-level functions, but if you feel it would help you to have more, feel free to suggest them.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085528194)
I prefer avoiding early returns if they don't really simplify the code.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085528194)
I prefer avoiding early returns if they don't really simplify the code.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085520213)
> This is pretty much identical to the logic in CompareMainOrder, should this be encapsulated in a single place as the source of truth (rather than having to ensure they stay equivalent)?
Good idea, done.
> Unrelated to this particular PR, but what is the intended caller/use case for CompareMainOrder? Is the reason CompareMainOrder calls MakeAcceptable while ChunkOrder does not simply because we assume its already been called when ChunkOrder is invoked?
`ChunkOrder`'s comparison functio
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2085520213)
> This is pretty much identical to the logic in CompareMainOrder, should this be encapsulated in a single place as the source of truth (rather than having to ensure they stay equivalent)?
Good idea, done.
> Unrelated to this particular PR, but what is the intended caller/use case for CompareMainOrder? Is the reason CompareMainOrder calls MakeAcceptable while ChunkOrder does not simply because we assume its already been called when ChunkOrder is invoked?
`ChunkOrder`'s comparison functio
...
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874185965)
Changes made to address @monlovesmango's comments:
```diff
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -282,6 +282,27 @@ private:
return a->m_sequence <=> b->m_sequence;
}
+ /** Compare two entries (which must both exist within the main graph). */
+ std::strong_ordering CompareMainTransactions(GraphIndex a, GraphIndex b) const noexcept
+ {
+ Assume(a < m_entries.size() && b < m_entries.size());
+ const auto& entry_a = m_entries[a];
+
...
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2874185965)
Changes made to address @monlovesmango's comments:
```diff
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -282,6 +282,27 @@ private:
return a->m_sequence <=> b->m_sequence;
}
+ /** Compare two entries (which must both exist within the main graph). */
+ std::strong_ordering CompareMainTransactions(GraphIndex a, GraphIndex b) const noexcept
+ {
+ Assume(a < m_entries.size() && b < m_entries.size());
+ const auto& entry_a = m_entries[a];
+
...
💬 ariel-lore commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874205808)
Concept NACK
Deprecating the feature is an indication that the feature could be removed in the future without discussion. It is not different enough to just removing the feature entirely today, thus is isn't a compromise. Some are saying that features can stay deprecated for years but that's a false equivalence because those features are not as controversial as the OP_RETURN limit.
Increasing the default OP_RETURN limit to 100,000 is also contentious and should not be merged without furthe
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2874205808)
Concept NACK
Deprecating the feature is an indication that the feature could be removed in the future without discussion. It is not different enough to just removing the feature entirely today, thus is isn't a compromise. Some are saying that features can stay deprecated for years but that's a false equivalence because those features are not as controversial as the OP_RETURN limit.
Increasing the default OP_RETURN limit to 100,000 is also contentious and should not be merged without furthe
...
💬 theStack commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2874221345)
Rebased on master (needed due to a few conflicts in functional tests after #32155 was merged).
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2874221345)
Rebased on master (needed due to a few conflicts in functional tests after #32155 was merged).
⚠️ Pamela001001 opened an issue: "HOW TO FIND A LEGITIMATE CRYPTO RECOVERY EXPERT//PASSCODE CYBER RECOVERY"
(https://github.com/bitcoin/bitcoin/issues/32474)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
PASSCODE CYBER RECOVERY
WhatsApp: +1(647)399-4074
Telegram : @passcodecyberrecovery
A clip titled “Joe Rogan Reveals Secret Ethereum Fork” autopay on YouTube, featuring Rogan’s unmistakable voice praising a supposed “ETH 2.0 upgrade” that promised early investors up to 10x returns. Everything about the video seemed authentic, Rogan's casual tone, the familiar podcast studio, and even comm
...
(https://github.com/bitcoin/bitcoin/issues/32474)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
PASSCODE CYBER RECOVERY
WhatsApp: +1(647)399-4074
Telegram : @passcodecyberrecovery
A clip titled “Joe Rogan Reveals Secret Ethereum Fork” autopay on YouTube, featuring Rogan’s unmistakable voice praising a supposed “ETH 2.0 upgrade” that promised early investors up to 10x returns. Everything about the video seemed authentic, Rogan's casual tone, the familiar podcast studio, and even comm
...
💬 Pamela001001 commented on issue "HOW TO FIND A LEGITIMATE CRYPTO RECOVERY EXPERT//PASSCODE CYBER RECOVERY":
(https://github.com/bitcoin/bitcoin/issues/32474#issuecomment-2874355189)
HOW TO FIND A LEGITIMATE CRYPTO RECOVERY EXPERT//PASSCODE CYBER RECOVERY
PASSCODE CYBER RECOVERY
WhatsApp: +1(647)399-4074
Telegram : @passcodecyberrecovery
A clip titled “Joe Rogan Reveals Secret Ethereum Fork” autopay on YouTube, featuring Rogan’s unmistakable voice praising a supposed “ETH 2.0 upgrade” that promised early investors up to 10x returns. Everything about the video seemed authentic, Rogan's casual tone, the familiar podcast studio, and even commentary from a so-called “blockchai
...
(https://github.com/bitcoin/bitcoin/issues/32474#issuecomment-2874355189)
HOW TO FIND A LEGITIMATE CRYPTO RECOVERY EXPERT//PASSCODE CYBER RECOVERY
PASSCODE CYBER RECOVERY
WhatsApp: +1(647)399-4074
Telegram : @passcodecyberrecovery
A clip titled “Joe Rogan Reveals Secret Ethereum Fork” autopay on YouTube, featuring Rogan’s unmistakable voice praising a supposed “ETH 2.0 upgrade” that promised early investors up to 10x returns. Everything about the video seemed authentic, Rogan's casual tone, the familiar podcast studio, and even commentary from a so-called “blockchai
...
✅ fanquake closed an issue: "HOW TO FIND A LEGITIMATE CRYPTO RECOVERY EXPERT//PASSCODE CYBER RECOVERY"
(https://github.com/bitcoin/bitcoin/issues/32474)
(https://github.com/bitcoin/bitcoin/issues/32474)