📝 hebasto opened a pull request: "msvc: Define the same `QT_...` macros as in Autotools builds"
(https://github.com/bitcoin/bitcoin/pull/29044)
There are no reasons to have such a diversion.
Also it fixes https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1847971114.
(https://github.com/bitcoin/bitcoin/pull/29044)
There are no reasons to have such a diversion.
Also it fixes https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1847971114.
💬 hebasto commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1848453153)
> It's something to do with `CMainSignals&`. Wherever it's used in libbitcoin_qt the msvc compiler generates a syntax error of:
>
> `Error C2238 unexpected token(s) preceding ';'`
Fixed in #29044.
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1848453153)
> It's something to do with `CMainSignals&`. Wherever it's used in libbitcoin_qt the msvc compiler generates a syntax error of:
>
> `Error C2238 unexpected token(s) preceding ';'`
Fixed in #29044.
💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1848453371)
Thanks for reviewing @martinus @maflcko
Forced pushed from https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 to https://github.com/bitcoin/bitcoin/commit/f8ef5dbc8471b711d233e3bec64597498a641252
to fix CI failure
[compare_changes](https://github.com/bitcoin/bitcoin/compare/fab46cc454f94b33dda6bc68a7234b439310f358..f8ef5dbc8471b711d233e3bec64597498a641252)
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1848453371)
Thanks for reviewing @martinus @maflcko
Forced pushed from https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 to https://github.com/bitcoin/bitcoin/commit/f8ef5dbc8471b711d233e3bec64597498a641252
to fix CI failure
[compare_changes](https://github.com/bitcoin/bitcoin/compare/fab46cc454f94b33dda6bc68a7234b439310f358..f8ef5dbc8471b711d233e3bec64597498a641252)
💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421441743)
Thanks @martinus Will review
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421441743)
Thanks @martinus Will review
📝 hebasto opened a pull request: "msvc: Optimize "Release" builds"
(https://github.com/bitcoin/bitcoin/pull/29045)
It is awkward not using optimization.
This PR reduces the duration of functional tests by an hour.
(https://github.com/bitcoin/bitcoin/pull/29045)
It is awkward not using optimization.
This PR reduces the duration of functional tests by an hour.
💬 hebasto commented on pull request "msvc: Optimize "Release" builds":
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848456277)
Friendly ping @sipsorcery :)
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848456277)
Friendly ping @sipsorcery :)
👍 hebasto approved a pull request: "util: Remove DirIsWritable, GetUniquePath"
(https://github.com/bitcoin/bitcoin/pull/28075#pullrequestreview-1773778627)
ACK fa3da629a1aebcb4500803d7417feed8e34285b0, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/28075#pullrequestreview-1773778627)
ACK fa3da629a1aebcb4500803d7417feed8e34285b0, I have reviewed the code and it looks OK.
💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1848591715)
@sipa Would you please review my PR? I really appreciate it.
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1848591715)
@sipa Would you please review my PR? I really appreciate it.
💬 martinus commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1848609143)
I updated my PR by removing the changes that I did in initializer-lists. These were not necessary, evaluation order in these is well defined.
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1848609143)
I updated my PR by removing the changes that I did in initializer-lists. These were not necessary, evaluation order in these is well defined.
💬 sipsorcery commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848766633)
@hebasto what version of QT are you using for Windows builds?
It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848766633)
@hebasto what version of QT are you using for Windows builds?
It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.
💬 hebasto commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848769593)
> @hebasto what version of QT are you using for Windows builds?
Qt 5.15.11
> It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.
I've made another Qt 5.15.11 build just today with no issues.
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848769593)
> @hebasto what version of QT are you using for Windows builds?
Qt 5.15.11
> It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.
I've made another Qt 5.15.11 build just today with no issues.
💬 sipsorcery commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848781032)
tACK 1a5dae630df1eef9eac51557b2f1c5dba0924953.
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848781032)
tACK 1a5dae630df1eef9eac51557b2f1c5dba0924953.
💬 sipsorcery commented on pull request "msvc: Optimize "Release" builds":
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848790814)
tACK 6e0f1d2abbb700d4fd4b956a7d1f9505b653653c.
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848790814)
tACK 6e0f1d2abbb700d4fd4b956a7d1f9505b653653c.
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421613674)
Leftover comment?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421613674)
Leftover comment?
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421615040)
Not sure if I'm missing something, but this only checks that the v3 inheritance rules are enforced among transactions in the package. What if someone sent a package of 2 v3 transactions, but the child transaction also spent a transaction that was already in the mempool? I think the `CheckV3Inheritance()` needs to be able to pull parents from both the mempool and from the package, perhaps?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421615040)
Not sure if I'm missing something, but this only checks that the v3 inheritance rules are enforced among transactions in the package. What if someone sent a package of 2 v3 transactions, but the child transaction also spent a transaction that was already in the mempool? I think the `CheckV3Inheritance()` needs to be able to pull parents from both the mempool and from the package, perhaps?
💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259)
This is an aside -- in the cluster mempool branch, I think I'd like to change this to just look at direct parents rather than ancestors (I believe that in the normal course of transaction validation, we should no longer need to calculate ancestor sets when we're not worried about ancestor limits or updating cached ancestor state anymore). Can you think of any reason it would be problematic to replace ancestors with just an entry's direct parents here?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259)
This is an aside -- in the cluster mempool branch, I think I'd like to change this to just look at direct parents rather than ancestors (I believe that in the normal course of transaction validation, we should no longer need to calculate ancestor sets when we're not worried about ancestor limits or updating cached ancestor state anymore). Can you think of any reason it would be problematic to replace ancestors with just an entry's direct parents here?
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1421668681)
I wouldn't say it's safe to ignore, but the idea is that we often want to be able to batch deletions, and then clean things up in one pass. So the call sites should all be dealing with this issue and ensuring that we always clean up at some point.
(This is definitely an area where I expect that we'll be re-engineering all this logic and trying to come up with a better abstraction layer so that this is more robust and easier to think about!)
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1421668681)
I wouldn't say it's safe to ignore, but the idea is that we often want to be able to batch deletions, and then clean things up in one pass. So the call sites should all be dealing with this issue and ensuring that we always clean up at some point.
(This is definitely an area where I expect that we'll be re-engineering all this logic and trying to come up with a better abstraction layer so that this is more robust and easier to think about!)
💬 kevkevinpal commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1848832142)
ACK [1553c80](https://github.com/bitcoin/bitcoin/pull/29037/commits/1553c8078698df1058b62e8fdadaf74160977b30)
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1848832142)
ACK [1553c80](https://github.com/bitcoin/bitcoin/pull/29037/commits/1553c8078698df1058b62e8fdadaf74160977b30)
💬 luke-jr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1848850110)
The vulnerability this fixes has been assigned CVE-2023-50428
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1848850110)
The vulnerability this fixes has been assigned CVE-2023-50428
💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1421680771)
Done
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1421680771)
Done