💬 Sjors commented on issue "wallets created on master get corrupted when processed with v25":
(https://github.com/bitcoin/bitcoin/issues/27915#issuecomment-1607644227)
I'll review the apostropocalypse fix soon, thanks.
(https://github.com/bitcoin/bitcoin/issues/27915#issuecomment-1607644227)
I'll review the apostropocalypse fix soon, thanks.
💬 MarcoFalke commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1607655279)
> I guess -debug=net would useful here, if you can ever reproduce it. It might give us a hint which message we were about to react to.
If you do this, make sure to add additional logging (maybe `nBytes`, `data.size()`, and `node.nSendOffset` after the `Send()`?) in the function that asserted, because there is none.
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1607655279)
> I guess -debug=net would useful here, if you can ever reproduce it. It might give us a hint which message we were about to react to.
If you do this, make sure to add additional logging (maybe `nBytes`, `data.size()`, and `node.nSendOffset` after the `Send()`?) in the function that asserted, because there is none.
💬 Sjors commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1607656508)
@kristapsk I removed two of the three hashboards and set the target consumption to 100W. It's practice it's more like 130, but that's fine, and not too noisy (mine came with BrainsOS+ installed)
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1607656508)
@kristapsk I removed two of the three hashboards and set the target consumption to 100W. It's practice it's more like 130, but that's fine, and not too noisy (mine came with BrainsOS+ installed)
💬 MarcoFalke commented on pull request "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee":
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607661240)
> We use the fee_a × vsize_b > fee_b × vsize_a trick in a bunch of places. On the other hand, fee is calculated from vsize × feerate. So, in fuzzing based on a feerate input, we should limit the feerate to prevent integer overflows with int64_t to max_feerate = int64_t_max / (max_vsize²)?
I'd say no, because the tricks use `double`, not `int64_t`, no? Also, this is an rpc interface bug, not a fuzzing bug, see https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1598231773
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607661240)
> We use the fee_a × vsize_b > fee_b × vsize_a trick in a bunch of places. On the other hand, fee is calculated from vsize × feerate. So, in fuzzing based on a feerate input, we should limit the feerate to prevent integer overflows with int64_t to max_feerate = int64_t_max / (max_vsize²)?
I'd say no, because the tricks use `double`, not `int64_t`, no? Also, this is an rpc interface bug, not a fuzzing bug, see https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1598231773
💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1607672356)
This needs rebase for CI to pass.
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1607672356)
This needs rebase for CI to pass.
💬 Xekyo commented on pull request "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee":
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607693447)
Oh, I recently also bumped into an integer overflow when fuzzing coin selection stuff due to feerates being too large, and I figured that the underlying issue was the same, I meant to reply to the “larger question”.
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607693447)
Oh, I recently also bumped into an integer overflow when fuzzing coin selection stuff due to feerates being too large, and I figured that the underlying issue was the same, I meant to reply to the “larger question”.
💬 Sjors commented on pull request "getblocktemplate improvements for segwit and sigops":
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1607709381)
> changes the BIP 145 semantics of "sigops".
How so?
From [BIP 145](https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki#user-content-Sigops):
> Sigops
> For templates with "segwit" enabled as a rule, the "sigoplimit" and "sigops" keys must use the new values as calculated in [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Sigops).
This PR leaves the "segwit" rule enabled. You just don't have to specify it: https://github.com/bitcoin/bitcoin/pull
...
(https://github.com/bitcoin/bitcoin/pull/27433#issuecomment-1607709381)
> changes the BIP 145 semantics of "sigops".
How so?
From [BIP 145](https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki#user-content-Sigops):
> Sigops
> For templates with "segwit" enabled as a rule, the "sigoplimit" and "sigops" keys must use the new values as calculated in [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Sigops).
This PR leaves the "segwit" rule enabled. You just don't have to specify it: https://github.com/bitcoin/bitcoin/pull
...
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242374407)
Fixed
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1242374407)
Fixed
💬 achow101 commented on issue "Unable to open descriptor wallets that are not in a wallet directory":
(https://github.com/bitcoin/bitcoin/issues/26739#issuecomment-1607725805)
> I could work on it if you both are happy with it since I do also agree with that feature.
I'm not sure that this is something that we should implement. This issue is primarily for brainstorming and comment gathering.
(https://github.com/bitcoin/bitcoin/issues/26739#issuecomment-1607725805)
> I could work on it if you both are happy with it since I do also agree with that feature.
I'm not sure that this is something that we should implement. This issue is primarily for brainstorming and comment gathering.
💬 vasild commented on pull request "net: run disconnect in I2P thread":
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1607731240)
I acknowledge the problem described in https://github.com/bitcoin/bitcoin/issues/27843: `ThreadI2PAcceptIncoming()` calls `CreateNodeFromAcceptedSocket()`. The latter checks `nInbound >= nMaxInbound` and calls `AttemptToEvictConnection()` if that is true, but the eviction is done asynchronously - `AttemptToEvictConnection()` only marks it for disconnect and leaves the actual disconnection and socket closure to another thread for a later time (`ThreadSocketHandler()`). Until that other thread act
...
(https://github.com/bitcoin/bitcoin/pull/27912#issuecomment-1607731240)
I acknowledge the problem described in https://github.com/bitcoin/bitcoin/issues/27843: `ThreadI2PAcceptIncoming()` calls `CreateNodeFromAcceptedSocket()`. The latter checks `nInbound >= nMaxInbound` and calls `AttemptToEvictConnection()` if that is true, but the eviction is done asynchronously - `AttemptToEvictConnection()` only marks it for disconnect and leaves the actual disconnection and socket closure to another thread for a later time (`ThreadSocketHandler()`). Until that other thread act
...
💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1607734897)
Updated `feature_taproot.py` tests
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1607734897)
Updated `feature_taproot.py` tests
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1242396610)
Marking this as resolved, let me know if you think it is not.
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1242396610)
Marking this as resolved, let me know if you think it is not.
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1607757096)
`b96bd52f3a...4557cc336f`: rebase
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1607757096)
`b96bd52f3a...4557cc336f`: rebase
💬 ryanofsky commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1607791383)
fad126dae2bc654a28b0a77e35aa43cae42d1e0c seems like a nice improvement. It makes existing MakeByteSpan function safer by only allowing it to be called on arrays of characters, and not array of other things like `uint16_t` that could have platform specific representations. Some thoughts though:
- `MakeByteSpan` probably deserves to have some documentation since it is one of the more useful function in that file and is called a lot of places.
- I don't like introducing the `ByteSpanCast` funct
...
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1607791383)
fad126dae2bc654a28b0a77e35aa43cae42d1e0c seems like a nice improvement. It makes existing MakeByteSpan function safer by only allowing it to be called on arrays of characters, and not array of other things like `uint16_t` that could have platform specific representations. Some thoughts though:
- `MakeByteSpan` probably deserves to have some documentation since it is one of the more useful function in that file and is called a lot of places.
- I don't like introducing the `ByteSpanCast` funct
...
⚠️ Crypt-iQ opened an issue: "depends build fails macOS intel"
(https://github.com/bitcoin/bitcoin/issues/27977)
Make command:
```
make CC="/usr/local/Cellar/llvm/16.0.4/bin/clang" CXX="/usr/local/Cellar/llvm/16.0.4/bin/clang++" LDFLAGS="-L/usr/local/opt/llvm@16/lib/c++ -Wl,-rpath,/usr/local/opt/llvm@16/lib/c++" NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 -j 4
```
Environment: macOS intel, clang16
Last lines of output:
```
echo Building native_ds_store...
mkdir -p /Users/nsa/bitcoin/depends/work/build/x86_64-apple-darwin21.6.0/native_ds_store/1.3.0-f3b721f4d37/.
{ cd /Users/nsa/bitcoin
...
(https://github.com/bitcoin/bitcoin/issues/27977)
Make command:
```
make CC="/usr/local/Cellar/llvm/16.0.4/bin/clang" CXX="/usr/local/Cellar/llvm/16.0.4/bin/clang++" LDFLAGS="-L/usr/local/opt/llvm@16/lib/c++ -Wl,-rpath,/usr/local/opt/llvm@16/lib/c++" NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 -j 4
```
Environment: macOS intel, clang16
Last lines of output:
```
echo Building native_ds_store...
mkdir -p /Users/nsa/bitcoin/depends/work/build/x86_64-apple-darwin21.6.0/native_ds_store/1.3.0-f3b721f4d37/.
{ cd /Users/nsa/bitcoin
...
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242445552)
sorry, I guess I misinterpreted this conversation. let's clarify :)
your initial comment had two suggestions
1. remove the out param & return optional instead of bool
2. drop the optional from the out param
it looks like I focused & responded to the issue with # 1, but overlooked # 2. to restate, I think # 1 would be nice in isolation, but based on the caller, doesn't make the most sense here.
that said, # 2 of dropping the optional seems reasonable. the caller is already respons
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1242445552)
sorry, I guess I misinterpreted this conversation. let's clarify :)
your initial comment had two suggestions
1. remove the out param & return optional instead of bool
2. drop the optional from the out param
it looks like I focused & responded to the issue with # 1, but overlooked # 2. to restate, I think # 1 would be nice in isolation, but based on the caller, doesn't make the most sense here.
that said, # 2 of dropping the optional seems reasonable. the caller is already respons
...
📝 ryanofsky opened a pull request: "refactor: Drop unsafe AsBytePtr function"
(https://github.com/bitcoin/bitcoin/pull/27978)
Replace calls to AsBytePtr with direct calls to reinterpret_cast. AsBytePtr is just a wrapper around reinterpret_cast. It accepts any type of pointer as an argument and uses reinterpret_cast to cast the argument to a std::byte pointer.
Despite taking any type of pointer as an argument, it is not useful to call AsBytePtr on most types of pointers, because byte representations of most types will be platform specific or undefined. Because it is named similarly to the AsBytes function, AsBytePtr
...
(https://github.com/bitcoin/bitcoin/pull/27978)
Replace calls to AsBytePtr with direct calls to reinterpret_cast. AsBytePtr is just a wrapper around reinterpret_cast. It accepts any type of pointer as an argument and uses reinterpret_cast to cast the argument to a std::byte pointer.
Despite taking any type of pointer as an argument, it is not useful to call AsBytePtr on most types of pointers, because byte representations of most types will be platform specific or undefined. Because it is named similarly to the AsBytes function, AsBytePtr
...
💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607849084)
> > the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp?rgh-link-date=2023-05-16T15%3A52%3A00Z#L1021).
>
> However, [Xcode 13 Release Notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-13-release-notes) state:
>
> > All programs and dylibs built with a deployment target of macOS 12 or iOS 15 or later now use the chained fixups format. This uses different load c
...
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607849084)
> > the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp?rgh-link-date=2023-05-16T15%3A52%3A00Z#L1021).
>
> However, [Xcode 13 Release Notes](https://developer.apple.com/documentation/xcode-release-notes/xcode-13-release-notes) state:
>
> > All programs and dylibs built with a deployment target of macOS 12 or iOS 15 or later now use the chained fixups format. This uses different load c
...
🤔 D33r-Gee reviewed a pull request: "Add menu option to migrate a wallet"
(https://github.com/bitcoin-core/gui/pull/738#pullrequestreview-1499005933)
tested ACK on WSL Ubuntu 22.04
Migrated a few test wallets using the GUI on regtest, both encrypted and not-encrypted wallet yelled successful results.
(https://github.com/bitcoin-core/gui/pull/738#pullrequestreview-1499005933)
tested ACK on WSL Ubuntu 22.04
Migrated a few test wallets using the GUI on regtest, both encrypted and not-encrypted wallet yelled successful results.
📝 Sultansaudtr opened a pull request: "Sultan.saud@mail.ru"
(https://github.com/bitcoin/bitcoin/pull/27979)
<!--
*** 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:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27979)
<!--
*** 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:
* Any test improvements or new tests that improv
...