💬 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
👋 fanquake's pull request is ready for review: "[26.x] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/28872)
(https://github.com/bitcoin/bitcoin/pull/28872)
💬 maflcko commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1814561402)
> NetBSD and FreeBSD, `127.1` is resolved correctly
Confirmed this on FreeBSD. I don't have OpenBSD or NetBSD.
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1814561402)
> NetBSD and FreeBSD, `127.1` is resolved correctly
Confirmed this on FreeBSD. I don't have OpenBSD or NetBSD.
💬 maflcko commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1395800664)
According to iwyu, it seems better to include
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1395800664)
According to iwyu, it seems better to include
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395809829)
I don't quite follow. `hdmasterfingerprint` is not available for descriptor wallets.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395809829)
I don't quite follow. `hdmasterfingerprint` is not available for descriptor wallets.
📝 hebasto opened a pull request: "Fix SSE4.1-related issues"
(https://github.com/bitcoin/bitcoin/pull/28893)
1. Fix the test for SSE4.1 intrinsics during build system configuration, which currently can be false positive, for example, when `CXXFLAGS="-mno-sse4.1"` provided.
This PR fixes the test by adding the `_mm_blend_epi16` SSE4.1 function used in our codebase.
2. Guard `sha_x86_shani.cpp` code with `ENABLE_SSE41` macro as it uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set.
It is possible that SHA-NI is enabled even when SSE4.1 is disabled, which causes compile errors
...
(https://github.com/bitcoin/bitcoin/pull/28893)
1. Fix the test for SSE4.1 intrinsics during build system configuration, which currently can be false positive, for example, when `CXXFLAGS="-mno-sse4.1"` provided.
This PR fixes the test by adding the `_mm_blend_epi16` SSE4.1 function used in our codebase.
2. Guard `sha_x86_shani.cpp` code with `ENABLE_SSE41` macro as it uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set.
It is possible that SHA-NI is enabled even when SSE4.1 is disabled, which causes compile errors
...
📝 furszy opened a pull request: "wallet: batch all atomic spkms setup db writes in a single db txn "
(https://github.com/bitcoin/bitcoin/pull/28894)
Work decoupled from #28574.
Instead of performing multiple single write operations per spkm
setup call, this PR batches them all within a single atomic db txn.
Speeding up the process and preventing the wallet from entering
an inconsistent state if any of the intermediate transactions fail
(which shouldn't happen but.. if it does, it is better to not store
any spkm rather than storing them partially).
To compare the changes, added benchmark in the first commit.
(https://github.com/bitcoin/bitcoin/pull/28894)
Work decoupled from #28574.
Instead of performing multiple single write operations per spkm
setup call, this PR batches them all within a single atomic db txn.
Speeding up the process and preventing the wallet from entering
an inconsistent state if any of the intermediate transactions fail
(which shouldn't happen but.. if it does, it is better to not store
any spkm rather than storing them partially).
To compare the changes, added benchmark in the first commit.
💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395831233)
Thanks for the explanations.
I've refactored out handling pushed data, as I wasn't enjoying all the nested if statements, and notice that we are currently handling "non-minimal pushes" the same as minimal pushes with non-minimally-encoded values, which is to use `OPCODE<hex>`. This seems ok, and it will not be a lossy decode, but do we want to differentiate between the two pathways to this decode format?
See first and last case in snippet to illustrate what I mean above:
```cpp
/*
*
...
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395831233)
Thanks for the explanations.
I've refactored out handling pushed data, as I wasn't enjoying all the nested if statements, and notice that we are currently handling "non-minimal pushes" the same as minimal pushes with non-minimally-encoded values, which is to use `OPCODE<hex>`. This seems ok, and it will not be a lossy decode, but do we want to differentiate between the two pathways to this decode format?
See first and last case in snippet to illustrate what I mean above:
```cpp
/*
*
...