💬 hebasto 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-1607542766)
Guix (self-signed) `bitcoin-qt` tested on macOS Ventura 13.4.1 (Apple M1).
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1607542766)
Guix (self-signed) `bitcoin-qt` tested on macOS Ventura 13.4.1 (Apple M1).
📝 MarcoFalke opened a pull request: " ci: Start with clean env "
(https://github.com/bitcoin/bitcoin/pull/27976)
Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564529747
(https://github.com/bitcoin/bitcoin/pull/27976)
Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564529747
💬 Xekyo commented on pull request "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee":
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607580462)
> Fixes #27913, although this does bring up a larger question of how we should handle such large feerates in fuzzing.
My understanding is that we should fuzz as broadly as possible, but our fuzzers should probably catch before overflows of `CAmount` occur. If I didn’t err on my napkin math that still allows us to do full block transactions at 92 ₿ per vB (9,223,372,036 ṩ/vB). So… maybe we just limit feerates to 9 billion ṩ/vB in fuzzers?
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607580462)
> Fixes #27913, although this does bring up a larger question of how we should handle such large feerates in fuzzing.
My understanding is that we should fuzz as broadly as possible, but our fuzzers should probably catch before overflows of `CAmount` occur. If I didn’t err on my napkin math that still allows us to do full block transactions at 92 ₿ per vB (9,223,372,036 ṩ/vB). So… maybe we just limit feerates to 9 billion ṩ/vB in fuzzers?
💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1607633650)
If I read correctly it happened _after_ IBD.
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.
As an aside, since you configured the wallet, be careful loading descriptor wallets on master until #27915 is resolved.
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1607633650)
If I read correctly it happened _after_ IBD.
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.
As an aside, since you configured the wallet, be careful loading descriptor wallets on master until #27915 is resolved.
💬 Xekyo commented on pull request "feerate: For GetFeePerK() return nSatoshisPerK instead of round trip through GetFee":
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607637305)
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²)`?
If we assume that our code does not have to deal with transactions greater than 100_000 vB:
`9_223_372_036_854_775_807÷(100_000²) = 922_337_203_685 [ṩ/kvB] ≈ 922_337_203 [ṩ/vB] ≈ 9_223 ₿/kvB`
If
...
(https://github.com/bitcoin/bitcoin/pull/27914#issuecomment-1607637305)
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²)`?
If we assume that our code does not have to deal with transactions greater than 100_000 vB:
`9_223_372_036_854_775_807÷(100_000²) = 922_337_203_685 [ṩ/kvB] ≈ 922_337_203 [ṩ/vB] ≈ 9_223 ₿/kvB`
If
...
💬 Sjors commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1607638095)
> While reviewing it, discovered that descriptors containing a key with an origin are actually incorrect.
Glad the tests were useful :-)
> The tests must pass in v25 with these commits.
Thanks, I was thinking of testing that as well, but couldn't figure out how to do so trivially with my own commit.
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1607638095)
> While reviewing it, discovered that descriptors containing a key with an origin are actually incorrect.
Glad the tests were useful :-)
> The tests must pass in v25 with these commits.
Thanks, I was thinking of testing that as well, but couldn't figure out how to do so trivially with my own commit.
💬 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
...