💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010256657)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010256657)
Fixed.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010257849)
I've replaced "maintains" with "defines". As far as the interface is concerned, what the actual implementation does in this regard is irrelevant.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010257849)
I've replaced "maintains" with "defines". As far as the interface is concerned, what the actual implementation does in this regard is irrelevant.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010258039)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010258039)
Fixed.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010260015)
I believe the plan is to basically run it at every "stable" point in time, so after any transaction or block processing.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010260015)
I believe the plan is to basically run it at every "stable" point in time, so after any transaction or block processing.
💬 l0rinc commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010256634)
According to https://www.reddit.com/r/cpp_questions/comments/plg8ij/expression_vs_statement/
> Statements do not have a return type
So by definition, unless they're empty, they cannot be `side-effect-free` - if my interpretation is correct.
```suggestion
expression is always evaluated. However, if the compiler can prove that
an expression inside `Assume` is side-effect-free, it may optimize the call away,
```
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010256634)
According to https://www.reddit.com/r/cpp_questions/comments/plg8ij/expression_vs_statement/
> Statements do not have a return type
So by definition, unless they're empty, they cannot be `side-effect-free` - if my interpretation is correct.
```suggestion
expression is always evaluated. However, if the compiler can prove that
an expression inside `Assume` is side-effect-free, it may optimize the call away,
```
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2748280390)
@rkrux pushed an update with another related direction. Ideally a new contributor can add a set of spenders under a new function, aggregate to the list, without having to touch the current walls of current spender making functions?
Let me know if I'm getting warmer.
(https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2748280390)
@rkrux pushed an update with another related direction. Ideally a new contributor can add a set of spenders under a new function, aggregate to the list, without having to touch the current walls of current spender making functions?
Let me know if I'm getting warmer.
🤔 rkrux reviewed a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2710562125)
Answering open comments.
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2710562125)
Answering open comments.
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r2010279314)
Interesting, I like these points quite a lot. Summarising below, please correct me if I misunderstand:
**We can avoid using defaults here completely by letting the corresponding `version` default trickle down from the RPC help into these functions here.**
As I checked in the call sites of `CreateTransaction`, this^ requires other transaction creation RPCs to accept a version param as well such as `createpsbt`, `walletcreatefundedpsbt`, `send`, `sendall`, which seems consistent with an earl
...
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r2010279314)
Interesting, I like these points quite a lot. Summarising below, please correct me if I misunderstand:
**We can avoid using defaults here completely by letting the corresponding `version` default trickle down from the RPC help into these functions here.**
As I checked in the call sites of `CreateTransaction`, this^ requires other transaction creation RPCs to accept a version param as well such as `createpsbt`, `walletcreatefundedpsbt`, `send`, `sendall`, which seems consistent with an earl
...
📝 hebasto opened a pull request: "build: Remove bitness suffix from Windows installer"
(https://github.com/bitcoin/bitcoin/pull/32132)
Since support for 32-bit Windows has been dropped, the suffix is no longer necessary.
(https://github.com/bitcoin/bitcoin/pull/32132)
Since support for 32-bit Windows has been dropped, the suffix is no longer necessary.
👍 instagibbs approved a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2710575039)
LGTM dfea1fae92a1fd8ba0bbf5d52a37baffa28f9f8f
I think having "fail-fast", developer-friendly tests is a fine motivation. And nothing stops me/you from porting novel things to the functional framework for additional coverage.
The additional sanity check added in first commit also helps protect against test regressions in the future.
Hand-checked the scripts, seems correct.
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2710575039)
LGTM dfea1fae92a1fd8ba0bbf5d52a37baffa28f9f8f
I think having "fail-fast", developer-friendly tests is a fine motivation. And nothing stops me/you from porting novel things to the functional framework for additional coverage.
The additional sanity check added in first commit also helps protect against test regressions in the future.
Hand-checked the scripts, seems correct.
💬 ismaelsadeeq commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010300389)
Fixed!
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010300389)
Fixed!
💬 l0rinc commented on pull request "build: Remove bitness suffix from Windows installer":
(https://github.com/bitcoin/bitcoin/pull/32132#discussion_r2010308034)
Does the message below (written 11 years ago) still make sense now that there's no "64 bit version Windows installer", just a "Windows installer"?
> MessageBox MB_OK|MB_ICONSTOP "Cannot install 64-bit version on a 32-bit system."
(https://github.com/bitcoin/bitcoin/pull/32132#discussion_r2010308034)
Does the message below (written 11 years ago) still make sense now that there's no "64 bit version Windows installer", just a "Windows installer"?
> MessageBox MB_OK|MB_ICONSTOP "Cannot install 64-bit version on a 32-bit system."
🤔 l0rinc reviewed a pull request: "build: Remove bitness suffix from Windows installer"
(https://github.com/bitcoin/bitcoin/pull/32132#pullrequestreview-2710614237)
utACK fb2b05b1259d3e69e6e675adfa30b429424c7625
(https://github.com/bitcoin/bitcoin/pull/32132#pullrequestreview-2710614237)
utACK fb2b05b1259d3e69e6e675adfa30b429424c7625
💬 darosior commented on pull request "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()":
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-2748345445)
Leaning toward Concept NACK.
OP is suggesting to provide more guarantees in our mocked `getsockname` than the one used in production. On its face i think the motivation is misguided and we should rather aim for having stricter tests to make sure our code is robust as well as to surface some edge case we might have not considered. If the reason for opening this PR is that the change in #31676 led to some fuzz crashes, then i would say it works as intended. Instead of working around the crash i
...
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-2748345445)
Leaning toward Concept NACK.
OP is suggesting to provide more guarantees in our mocked `getsockname` than the one used in production. On its face i think the motivation is misguided and we should rather aim for having stricter tests to make sure our code is robust as well as to surface some edge case we might have not considered. If the reason for opening this PR is that the change in #31676 led to some fuzz crashes, then i would say it works as intended. Instead of working around the crash i
...
💬 l0rinc commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2748351872)
ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2748351872)
ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed
⚠️ mahmoudeA opened an issue: "Mahmoud amin elgder"
(https://github.com/bitcoin-core/gui/issues/859)
https://github.com/bitcoin-core/gui/blob/770d39a37652d40885533fecce37e9f71cc0d051/src/test/README.md?plain=1#L25
(https://github.com/bitcoin-core/gui/issues/859)
https://github.com/bitcoin-core/gui/blob/770d39a37652d40885533fecce37e9f71cc0d051/src/test/README.md?plain=1#L25
✅ mahmoudeA closed an issue: "Mahmoud amin elgder"
(https://github.com/bitcoin-core/gui/issues/859)
(https://github.com/bitcoin-core/gui/issues/859)
📝 mahmoudeA opened a pull request: "Mahmoud amin elgder Update libbitcoinkernel.pc.in"
(https://github.com/bitcoin-core/gui/pull/860)
https://github.com/bitcoin-core/gui/issues/859
<!--
*** 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:
...
(https://github.com/bitcoin-core/gui/pull/860)
https://github.com/bitcoin-core/gui/issues/859
<!--
*** 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:
...
✅ hebasto closed a pull request: "Mahmoud amin elgder Update libbitcoinkernel.pc.in"
(https://github.com/bitcoin-core/gui/pull/860)
(https://github.com/bitcoin-core/gui/pull/860)
📝 hebasto locked a pull request: "Mahmoud amin elgder Update libbitcoinkernel.pc.in"
(https://github.com/bitcoin-core/gui/pull/860)
https://github.com/bitcoin-core/gui/issues/859
<!--
*** 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:
...
(https://github.com/bitcoin-core/gui/pull/860)
https://github.com/bitcoin-core/gui/issues/859
<!--
*** 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:
...