💬 maflcko commented on pull request "RFC: Remove boost usage from kernel api / headers":
(https://github.com/bitcoin/bitcoin/pull/28335#discussion_r1395662791)
Thanks, I guess it could be moved to https://github.com/bitcoin/bitcoin/pull/28886 as well, if you don't mind and have to retouch for other reasons.
(https://github.com/bitcoin/bitcoin/pull/28335#discussion_r1395662791)
Thanks, I guess it could be moved to https://github.com/bitcoin/bitcoin/pull/28886 as well, if you don't mind and have to retouch for other reasons.
💬 m3dwards commented on issue "26.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1814401058)
@D33r-Gee one of the pieces of feedback I received was to differentiate between when it was asking the user to enter a command and when it was showing the result of a command. It was suggested to use $ to make the distinction.
Noted your commend and will see what others think.
(https://github.com/bitcoin/bitcoin/issues/28866#issuecomment-1814401058)
@D33r-Gee one of the pieces of feedback I received was to differentiate between when it was asking the user to enter a command and when it was showing the result of a command. It was suggested to use $ to make the distinction.
Noted your commend and will see what others think.
💬 maflcko commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#discussion_r1395665231)
```suggestion
for (const CTxMemPoolEntry& entry : GetEntrySet(setParentTransactions)) {
```
nit: Not a pointer?
(https://github.com/bitcoin/bitcoin/pull/28886#discussion_r1395665231)
```suggestion
for (const CTxMemPoolEntry& entry : GetEntrySet(setParentTransactions)) {
```
nit: Not a pointer?
💬 TheCharlatan commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#discussion_r1395671439)
The `p` is for `parent`? Just like before in `pit`?
(https://github.com/bitcoin/bitcoin/pull/28886#discussion_r1395671439)
The `p` is for `parent`? Just like before in `pit`?
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1814426649)
> Guix hashes for arm64-apple-darwin differs across the build platforms, unfortunately.
Looks like the difference is only in `bitcoin-qt`. I'll check if reverting #28778 (dropping the -O1 workaround) makes things deterministic again. That would be disappointing, and a bit confusing, because it'd mean that things were non-deterministic, then became deterministic with newer Clang, and then went non-deterministic again with an even newer Clang..
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1814426649)
> Guix hashes for arm64-apple-darwin differs across the build platforms, unfortunately.
Looks like the difference is only in `bitcoin-qt`. I'll check if reverting #28778 (dropping the -O1 workaround) makes things deterministic again. That would be disappointing, and a bit confusing, because it'd mean that things were non-deterministic, then became deterministic with newer Clang, and then went non-deterministic again with an even newer Clang..
🤔 stickies-v reviewed a pull request: "build: Introduce internal kernel library"
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1734365185)
Concept ACK
It turned out to be a bit less clean than I'd hoped, but I made this branch with a scripted-diff (with a second commit to re-order includes alphabetically) as an alternative to the manual first commit. I'm not sure it makes reviewing easier (and it needs some more manual edits to Makefile, ubsan and an unrelated include), but at least it's a helpful check:
https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:182d54aed91bfb1c161033642c322a542c8f2e04
```sh
%
...
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1734365185)
Concept ACK
It turned out to be a bit less clean than I'd hoped, but I made this branch with a scripted-diff (with a second commit to re-order includes alphabetically) as an alternative to the manual first commit. I'm not sure it makes reviewing easier (and it needs some more manual edits to Makefile, ubsan and an unrelated include), but at least it's a helpful check:
https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:182d54aed91bfb1c161033642c322a542c8f2e04
```sh
%
...
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1814434680)
The diff I see between the two bins is:
```diff
--- a.txt
+++ b.txt
@@ -1,8 +1,8 @@
-bitcoin-f387bc0783ba_aarch64/bin/bitcoin-qt:
+bitcoin-f387bc0783ba_x86_64/bin/bitcoin-qt:
(__TEXT,__text) section
0000000100017364 bti c
0000000100017368 sub sp, sp, #0x30
000000010001736c stp x20, x19, [sp, #0x10]
0000000100017370 stp x29, x30, [sp, #0x20]
0000000100017374 add x29, sp, #0x20
0000000100017378 mov x19, x0
@@ -2987768,31 +2987768,31 @@
0000000100b7cf38 sub w9, w9, w8
00000
...
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1814434680)
The diff I see between the two bins is:
```diff
--- a.txt
+++ b.txt
@@ -1,8 +1,8 @@
-bitcoin-f387bc0783ba_aarch64/bin/bitcoin-qt:
+bitcoin-f387bc0783ba_x86_64/bin/bitcoin-qt:
(__TEXT,__text) section
0000000100017364 bti c
0000000100017368 sub sp, sp, #0x30
000000010001736c stp x20, x19, [sp, #0x10]
0000000100017370 stp x29, x30, [sp, #0x20]
0000000100017374 add x29, sp, #0x20
0000000100017378 mov x19, x0
@@ -2987768,31 +2987768,31 @@
0000000100b7cf38 sub w9, w9, w8
00000
...
💬 dergoegge commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814449370)
> I am running it on my servers and I agree it should be improved.
@maflcko thoughts on deleting it?
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814449370)
> I am running it on my servers and I agree it should be improved.
@maflcko thoughts on deleting it?
💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814453982)
No objection to deleting it, once there is a replacement. Though, it seems there is no rush to delete it, before there is a replacement.
Maybe https://github.com/bitcoin/bitcoin/pull/28578 (fuzz: add target for DescriptorScriptPubKeyMan by brunoerg) could be reviewed, and confirmed that it can also catch the regression, or be adjusted to catch the regression?
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814453982)
No objection to deleting it, once there is a replacement. Though, it seems there is no rush to delete it, before there is a replacement.
Maybe https://github.com/bitcoin/bitcoin/pull/28578 (fuzz: add target for DescriptorScriptPubKeyMan by brunoerg) could be reviewed, and confirmed that it can also catch the regression, or be adjusted to catch the regression?
💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1814468723)
Re https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1734365185
> nit: in https://github.com/bitcoin/bitcoin/commit/be0b6233ec6cd21e5730380a3bc7f868189dabc9: is there a reason we move hex_base.{h, cpp} to crypto, and not consensus? It seems unrelated to our crypto library.
Currently both the `consensus` and `util` library share the `strencodings` files. This is not ideal, because the file will then have to be compiled twice (once for each of them). Further, the `util` library
...
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1814468723)
Re https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1734365185
> nit: in https://github.com/bitcoin/bitcoin/commit/be0b6233ec6cd21e5730380a3bc7f868189dabc9: is there a reason we move hex_base.{h, cpp} to crypto, and not consensus? It seems unrelated to our crypto library.
Currently both the `consensus` and `util` library share the `strencodings` files. This is not ideal, because the file will then have to be compiled twice (once for each of them). Further, the `util` library
...
💬 ryanofsky commented on pull request "multiprocess compatibility updates":
(https://github.com/bitcoin/bitcoin/pull/28721#issuecomment-1814515435)
Thanks for the reviews! I'll rebase #10102 now that this is merged, and create a new PR with the next set of changes from #10102
(https://github.com/bitcoin/bitcoin/pull/28721#issuecomment-1814515435)
Thanks for the reviews! I'll rebase #10102 now that this is merged, and create a new PR with the next set of changes from #10102
📝 maflcko opened a pull request: " refactor: P2P transport without serialize version and type "
(https://github.com/bitcoin/bitcoin/pull/28892)
Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.
This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.
(https://github.com/bitcoin/bitcoin/pull/28892)
Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.
This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1814523985)
> I'll check if reverting https://github.com/bitcoin/bitcoin/pull/28778 (dropping the -O1 workaround) makes things deterministic again.
It does not. Building this branch: https://github.com/fanquake/bitcoin/tree/llvm_17_0_5_macos_deps_test, still results in non-deterministic binaries.
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1814523985)
> I'll check if reverting https://github.com/bitcoin/bitcoin/pull/28778 (dropping the -O1 workaround) makes things deterministic again.
It does not. Building this branch: https://github.com/fanquake/bitcoin/tree/llvm_17_0_5_macos_deps_test, still results in non-deterministic binaries.
💬 maflcko commented on pull request "refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH":
(https://github.com/bitcoin/bitcoin/pull/28451#discussion_r1395767105)
Probably doesn't make much sense to touch the same line of code twice. https://github.com/bitcoin/bitcoin/pull/28892 is an alternative that removes this in one go. So I'll leave this pull in draft for now.
(https://github.com/bitcoin/bitcoin/pull/28451#discussion_r1395767105)
Probably doesn't make much sense to touch the same line of code twice. https://github.com/bitcoin/bitcoin/pull/28892 is an alternative that removes this in one go. So I'll leave this pull in draft for now.
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395768687)
Yes.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395768687)
Yes.
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1814537256)
> `CVectorWriter` to `VectorWriter`, drop version from `SpanReader`, allow psbt's to be serialized via `DataStream`; Drop CDataStream; drop GetVersion(), GetType() ?
I did `VectorWriter` as part of https://github.com/bitcoin/bitcoin/pull/28892, which seems already large enough to be its own pull. The rest can be done in a follow-up, I guess.
> Drop -rpcserialversion and the without_witness params to TxToUniv and EncodeHexTx
Done in https://github.com/bitcoin/bitcoin/pull/28890
> CAut
...
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1814537256)
> `CVectorWriter` to `VectorWriter`, drop version from `SpanReader`, allow psbt's to be serialized via `DataStream`; Drop CDataStream; drop GetVersion(), GetType() ?
I did `VectorWriter` as part of https://github.com/bitcoin/bitcoin/pull/28892, which seems already large enough to be its own pull. The rest can be done in a follow-up, I guess.
> Drop -rpcserialversion and the without_witness params to TxToUniv and EncodeHexTx
Done in https://github.com/bitcoin/bitcoin/pull/28890
> CAut
...
💬 jonatack commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#discussion_r1395782296)
I think you can drop lines 120-121 (and it's the first rather than second call on those IIUC).
Perhaps just name the platform in question.
```suggestion
// OpenBSD doesn't support the IPv4 shorthand notation with omitted zero-bytes.
```
(https://github.com/bitcoin/bitcoin/pull/28891#discussion_r1395782296)
I think you can drop lines 120-121 (and it's the first rather than second call on those IIUC).
Perhaps just name the platform in question.
```suggestion
// OpenBSD doesn't support the IPv4 shorthand notation with omitted zero-bytes.
```
🤔 jonatack reviewed a pull request: "test: fix `AddNode` unit test failure on OpenBSD"
(https://github.com/bitcoin/bitcoin/pull/28891#pullrequestreview-1734526928)
Concept ACK (is there a reason not to have a BSD CI task?)
(https://github.com/bitcoin/bitcoin/pull/28891#pullrequestreview-1734526928)
Concept ACK (is there a reason not to have a BSD CI task?)
🤔 BrandonOdiwuor reviewed a pull request: "wallet: propagete `checkChainLimits` error message to wallet"
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1734532113)
The PR looks good, left a Nit comment
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1734532113)
The PR looks good, left a Nit comment
💬 BrandonOdiwuor commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1395785522)
Nit: I believe this addition might be unnecessary, considering that other instances of `std::string` in the file seem to work fine without it
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1395785522)
Nit: I believe this addition might be unnecessary, considering that other instances of `std::string` in the file seem to work fine without it