✅ fanquake closed an issue: "Hello"
(https://github.com/bitcoin/bitcoin/issues/29273)
(https://github.com/bitcoin/bitcoin/issues/29273)
:lock: fanquake locked an issue: "Hello"
(https://github.com/bitcoin/bitcoin/issues/29273)
(https://github.com/bitcoin/bitcoin/issues/29273)
💬 0xB10C commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457097947)
nit: I think this tracepoint would be easier to reason about if it was one line per argument. Not sure if this PR is the right place to reformat it.
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457097947)
nit: I think this tracepoint would be easier to reason about if it was one line per argument. Not sure if this PR is the right place to reformat it.
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-1898030061)
`20a9fc83bd...cd5bbb12e0`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-1898030061)
`20a9fc83bd...cd5bbb12e0`: rebase due to conflicts
💬 0xB10C commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457111240)
```
(mocktwallet/spend.cpp:1339:5: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
#0 0x5589c92846e6 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1339:5
#1 0x5589c8d8617d in wallet::spend_tests::wallet_duplicated_preset_
...
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457111240)
```
(mocktwallet/spend.cpp:1339:5: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
#0 0x5589c92846e6 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1339:5
#1 0x5589c8d8617d in wallet::spend_tests::wallet_duplicated_preset_
...
💬 0xB10C commented on issue "Timeout downloading block":
(https://github.com/bitcoin/bitcoin/issues/12291#issuecomment-1898052245)
Is this fixed with https://github.com/bitcoin/bitcoin/pull/27626?
cc @instagibbs
(https://github.com/bitcoin/bitcoin/issues/12291#issuecomment-1898052245)
Is this fixed with https://github.com/bitcoin/bitcoin/pull/27626?
cc @instagibbs
💬 ArmchairCryptologist commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898054824)
As far as I can tell, the default value for `datacarriersize` has always been named `MAX_OP_RETURN_RELAY`, so claiming that it was intended to be applicable to anything else seems like a bit of stretch.
It's not that people are disagreeing that storing cat pictures on the blockchain is a bad idea, they disagree with the proposed way to limit it, as it would be a game of whack-a-mole. If you wanted to specifically target people using the segwit discount to store data cheaply in general, a bet
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898054824)
As far as I can tell, the default value for `datacarriersize` has always been named `MAX_OP_RETURN_RELAY`, so claiming that it was intended to be applicable to anything else seems like a bit of stretch.
It's not that people are disagreeing that storing cat pictures on the blockchain is a bad idea, they disagree with the proposed way to limit it, as it would be a game of whack-a-mole. If you wanted to specifically target people using the segwit discount to store data cheaply in general, a bet
...
💬 ArmchairCryptologist commented on issue "Timeout downloading block":
(https://github.com/bitcoin/bitcoin/issues/12291#issuecomment-1898097298)
Looks good to me. Grepping the debug logs, I'm not seeing any block download timeouts on my public nodes with ~500 incoming connections each since updating to 26.0 early December, except for once on one node, which occurred shortly after startup and before the mempool had finished loading. It looks like it didn't have a lot of peers when the block arrived, so that's probably not a real concern.
(https://github.com/bitcoin/bitcoin/issues/12291#issuecomment-1898097298)
Looks good to me. Grepping the debug logs, I'm not seeing any block download timeouts on my public nodes with ~500 incoming connections each since updating to 26.0 early December, except for once on one node, which occurred shortly after startup and before the mempool had finished loading. It looks like it didn't have a lot of peers when the block arrived, so that's probably not a real concern.
💬 fanquake commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1898118752)
Looks like the improvement is a bit more pronouced for x86_64, at least when using Clang (17.0.6). For example, comparing `MurmurHash3`, master (left), and this PR (right), we get less code and more `rol` instructions: https://www.diffchecker.com/I7s5JeTr/. For GCC 13.2, this PR makes no difference to the generated assembly: https://www.diffchecker.com/nM5DgKfN/.
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1898118752)
Looks like the improvement is a bit more pronouced for x86_64, at least when using Clang (17.0.6). For example, comparing `MurmurHash3`, master (left), and this PR (right), we get less code and more `rol` instructions: https://www.diffchecker.com/I7s5JeTr/. For GCC 13.2, this PR makes no difference to the generated assembly: https://www.diffchecker.com/nM5DgKfN/.
🚀 fanquake merged a pull request: "refactor: C++20: Use std::rotl"
(https://github.com/bitcoin/bitcoin/pull/29085)
(https://github.com/bitcoin/bitcoin/pull/29085)
💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-1898145002)
Rebased to use the new logging functions `LogInfo` and `LogDebug` from #28318 (see `developer-notes.md`).
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-1898145002)
Rebased to use the new logging functions `LogInfo` and `LogDebug` from #28318 (see `developer-notes.md`).
💬 glozow commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457194322)
nit: replace magic number with `MAX_STANDARD_TX_WEIGHT`
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457194322)
nit: replace magic number with `MAX_STANDARD_TX_WEIGHT`
💬 glozow commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457200003)
Would this be giving up too early? We could still find a different solution further down the function, no?
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457200003)
Would this be giving up too early? We could still find a different solution further down the function, no?
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1898164704)
Hm the CI failure of `previous releases, qt5 dev package and depends packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four days ago seems weird. Didn't see any conflicts when rebasing.
Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed the discurage -> discourage misspelling. I thought I did it in an earlier push.
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1898164704)
Hm the CI failure of `previous releases, qt5 dev package and depends packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four days ago seems weird. Didn't see any conflicts when rebasing.
Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed the discurage -> discourage misspelling. I thought I did it in an earlier push.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1898213458)
Thank you for looking this over @ryanofsky,
Updated 487b12e18ce007379a997da48b5ee97f459f6342 -> e3592ee55756c08bda3075b54cbff719d3fb964f ([noGlobalSignals_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_3) -> [noGlobalSignals_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_3..noGlobalSignals_4))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#issuec
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1898213458)
Thank you for looking this over @ryanofsky,
Updated 487b12e18ce007379a997da48b5ee97f459f6342 -> e3592ee55756c08bda3075b54cbff719d3fb964f ([noGlobalSignals_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_3) -> [noGlobalSignals_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_3..noGlobalSignals_4))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#issuec
...
💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457258511)
I think wrapping the trace parameter in `static_cast<int32_t>()` is the correct way to resolve this because `change_pos` is otherwise a `std::optional<unsigned int>`. In tracing.md `change_pos` is defined as int32 so it should always be returned as a signed 32-bit int.
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457258511)
I think wrapping the trace parameter in `static_cast<int32_t>()` is the correct way to resolve this because `change_pos` is otherwise a `std::optional<unsigned int>`. In tracing.md `change_pos` is defined as int32 so it should always be returned as a signed 32-bit int.
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898282534)
> > Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
>
> what do we mean by ravaging? afaik the biggest unnecessary bloater of the UTXO set is the stacks protocol.
Normal users only see what gets highlighted in famous explorers and discussed on twitter. I think the best way to "filter" everything is `blocksonly=1` as suggested by a few other developers.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898282534)
> > Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
>
> what do we mean by ravaging? afaik the biggest unnecessary bloater of the UTXO set is the stacks protocol.
Normal users only see what gets highlighted in famous explorers and discussed on twitter. I think the best way to "filter" everything is `blocksonly=1` as suggested by a few other developers.
💬 stickies-v commented on pull request "[26.x] more backports":
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1898291449)
Backports LGTM d055adad985752c5ae3cf9dd0f509c83850e6c5b but it seems that the commit messages are different than usual? It doesn't seem like we have formal documentation on this, but keeping things uniform helps keep scripts simple (e.g. compiling a list of all pulls that are backported from `git log`) so this might be worth updating?
- `Rebased-From` only mentions the SHA when I think typically we use the full hash (needed for `git fetch`)
- `Github-Pull` uses `bitcoin#<number>` when I think
...
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1898291449)
Backports LGTM d055adad985752c5ae3cf9dd0f509c83850e6c5b but it seems that the commit messages are different than usual? It doesn't seem like we have formal documentation on this, but keeping things uniform helps keep scripts simple (e.g. compiling a list of all pulls that are backported from `git log`) so this might be worth updating?
- `Rebased-From` only mentions the SHA when I think typically we use the full hash (needed for `git fetch`)
- `Github-Pull` uses `bitcoin#<number>` when I think
...
💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457305630)
.. or maybe `static_cast` is overkill, a cast with `int32_t()` seems to be what's recommended in developer-nodes.md .
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1457305630)
.. or maybe `static_cast` is overkill, a cast with `int32_t()` seems to be what's recommended in developer-nodes.md .
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898302003)
I got to ask, as even a maintainer has brought this up. What is stopping the demand for P2MS from moving to P2PK? IMO this solution is ineffective due to a lack of comprehensiveness.
*insert meme of gate with no fence*
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898302003)
I got to ask, as even a maintainer has brought this up. What is stopping the demand for P2MS from moving to P2PK? IMO this solution is ineffective due to a lack of comprehensiveness.
*insert meme of gate with no fence*