💬 achow101 commented on pull request "init: completely remove `-maxorphantx` option":
(https://github.com/bitcoin/bitcoin/pull/33872#issuecomment-3544705489)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
(https://github.com/bitcoin/bitcoin/pull/33872#issuecomment-3544705489)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
🚀 achow101 merged a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872)
(https://github.com/bitcoin/bitcoin/pull/33872)
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544786316)
Friendly ping @instagibbs @glozow! in [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430443357), I think we are testing the prioritization feature so its fine if we use `invalidateblock` here. We would only want to replace `invalidateblock` during re-org testing to use fork based approach.
Why invalidateblock is OK here? It's just a test utility mechanism to quickly return a mined transaction to the mempool to verify prioritization settings persist.
Line 183 - 188:
...
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544786316)
Friendly ping @instagibbs @glozow! in [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430443357), I think we are testing the prioritization feature so its fine if we use `invalidateblock` here. We would only want to replace `invalidateblock` during re-org testing to use fork based approach.
Why invalidateblock is OK here? It's just a test utility mechanism to quickly return a mined transaction to the mempool to verify prioritization settings persist.
Line 183 - 188:
...
💬 ajtowns commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3544844062)
> Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.
`Solver()` calls `MatchMultisig()` which invokes `num_keys = GetScriptNuymber(opcode, data, required_sigs, MAX_PUBKEYS_PER_MULTISIG)` which returns nullopt if the CScriptNum isn't minimal and between `required_sigs` and `MAX_PUBKEYS_..`, so I believe it's enforced in both places.
> Just to be clear, we cannot allow hybrid pubkeys to be processed by `CHECKMULTISIG`
...
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3544844062)
> Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.
`Solver()` calls `MatchMultisig()` which invokes `num_keys = GetScriptNuymber(opcode, data, required_sigs, MAX_PUBKEYS_PER_MULTISIG)` which returns nullopt if the CScriptNum isn't minimal and between `required_sigs` and `MAX_PUBKEYS_..`, so I believe it's enforced in both places.
> Just to be clear, we cannot allow hybrid pubkeys to be processed by `CHECKMULTISIG`
...
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544897954)
Thanks @theStack, Fixed `create_empty_fork` in `mempool_updatefromblock.py` moving from second commit to first.
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544897954)
Thanks @theStack, Fixed `create_empty_fork` in `mempool_updatefromblock.py` moving from second commit to first.
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536251389)
Placement doesn't matter, but I don't think moving closer to COMMANDS, REGISTER_COMMANDS or CLI_COMMANDS really makes sense -- COMMAND_OPTIONS relates to `argsman.AddCommand()` commands (used by bitcoin-wallet and bitcoin-util), which differ from `COMMANDS` (used by bitcoin-tx), `REGISTER_COMMANDS` (also used by bitcoin-tx), and `CLI_COMMANDS` (used by bitcoin-cli for its non-rpc commands, `-generate`, `-netinfo`, `-addrinfo`, `-getinfo`).
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536251389)
Placement doesn't matter, but I don't think moving closer to COMMANDS, REGISTER_COMMANDS or CLI_COMMANDS really makes sense -- COMMAND_OPTIONS relates to `argsman.AddCommand()` commands (used by bitcoin-wallet and bitcoin-util), which differ from `COMMANDS` (used by bitcoin-tx), `REGISTER_COMMANDS` (also used by bitcoin-tx), and `CLI_COMMANDS` (used by bitcoin-cli for its non-rpc commands, `-generate`, `-netinfo`, `-addrinfo`, `-getinfo`).
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536254793)
It's a docstring, so shows up in the doxygen docs https://doxygen.bitcoincore.org/class_args_manager.html#ab71d531303f7e6fe3bbf67683a414a2e ; maybe it's not that useful, but doesn't seem worth removing outright
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536254793)
It's a docstring, so shows up in the doxygen docs https://doxygen.bitcoincore.org/class_args_manager.html#ab71d531303f7e6fe3bbf67683a414a2e ; maybe it's not that useful, but doesn't seem worth removing outright
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536256619)
It's looked up by the key in `CheckCommandOptions`, so sorting seems fine to me?
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536256619)
It's looked up by the key in `CheckCommandOptions`, so sorting seems fine to me?
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536272651)
`FormatParagraph()` only indenting when it sees a `\n` seems plausible for dealing with an indented continuation (ie, `"Hello " + FormatParagraph("world!\n", 79, 4)`) and the current behaviour is tested, so I'd rather not touch that.
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536272651)
`FormatParagraph()` only indenting when it sees a `\n` seems plausible for dealing with an indented continuation (ie, `"Hello " + FormatParagraph("world!\n", 79, 4)`) and the current behaviour is tested, so I'd rather not touch that.
💬 lovepamthailand1672538 commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3545028414)
bc1qnppvarg8f0gzcfhmytuky0l5ncc90e5kyuxl8q
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3545028414)
bc1qnppvarg8f0gzcfhmytuky0l5ncc90e5kyuxl8q
💬 lovepamthailand1672538 commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3545029569)
bc1qnppvarg8f0gzcfhmytuky0l5ncc90e5kyuxl8q
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3545029569)
bc1qnppvarg8f0gzcfhmytuky0l5ncc90e5kyuxl8q
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536361107)
Not currently, I think
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536361107)
Not currently, I think
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536364679)
Added some unit test coverage
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2536364679)
Added some unit test coverage
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3545163344)
> If you can, I would appreciate adding slightly more context to the PR description and the commit messages.
Sorry, these seem completely self-explanatory to me, so I don't see what to add. Happy to answer questions or take suggestions though.
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3545163344)
> If you can, I would appreciate adding slightly more context to the PR description and the commit messages.
Sorry, these seem completely self-explanatory to me, so I don't see what to add. Happy to answer questions or take suggestions though.
💬 ajtowns commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3545336207)
> Unless we overhaul the sv2 [NewTemplate message](https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client) it doesn't help
Yes, I'm saying I think `NewTemplate` (and `CoinbaseOutputConstraints`) should be overhauled, to avoid sv2 software generating or modifying the coinbase, beyond adjusting the values in a reserved extranonce area of known/fixed size. (Parsing the coinbase tx is still necessary for the pool to determine whether to accept a
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3545336207)
> Unless we overhaul the sv2 [NewTemplate message](https://stratumprotocol.org/specification/07-Template-Distribution-Protocol/#72-newtemplate-server-client) it doesn't help
Yes, I'm saying I think `NewTemplate` (and `CoinbaseOutputConstraints`) should be overhauled, to avoid sv2 software generating or modifying the coinbase, beyond adjusting the values in a reserved extranonce area of known/fixed size. (Parsing the coinbase tx is still necessary for the pool to determine whether to accept a
...
💬 yuvicc commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#issuecomment-3545630672)
Concept ACK
[Wrote](https://github.com/yuvicc/test-bitcoinkernel/blob/master/context_free.c) a small program to test `btck_PrecomputedTransactionData`.
(https://github.com/bitcoin/bitcoin/pull/33891#issuecomment-3545630672)
Concept ACK
[Wrote](https://github.com/yuvicc/test-bitcoinkernel/blob/master/context_free.c) a small program to test `btck_PrecomputedTransactionData`.
✅ maflcko closed a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442)
(https://github.com/bitcoin/bitcoin/pull/30442)
📝 maflcko reopened a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
The in-memory representation of the UTXO set uses (salted) [SipHash](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L226) for avoiding key collision attacks.
Hashing `uint256` keys is performed frequently throughout the codebase. Previously, specialized optimizations existed as standalone functions (`SipHashUint256` and `SipHashUint25
...
(https://github.com/bitcoin/bitcoin/pull/30442)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
The in-memory representation of the UTXO set uses (salted) [SipHash](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L226) for avoiding key collision attacks.
Hashing `uint256` keys is performed frequently throughout the codebase. Previously, specialized optimizations existed as standalone functions (`SipHashUint256` and `SipHashUint25
...
📝 maflcko opened a pull request: "clang-format: Set InsertNewlineAtEOF: true"
(https://github.com/bitcoin/bitcoin/pull/33896)
Now that the minimum supported clang version is 17, the `InsertNewlineAtEOF` setting can be set to `true` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof)
This is in line with the already existing newline linter. Can be tested via:
```
truncate -s -1 src/init.cpp
git diff
# Should fail:
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=trailing_newline
# Restore newline:
git diff -U0
...
(https://github.com/bitcoin/bitcoin/pull/33896)
Now that the minimum supported clang version is 17, the `InsertNewlineAtEOF` setting can be set to `true` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof)
This is in line with the already existing newline linter. Can be tested via:
```
truncate -s -1 src/init.cpp
git diff
# Should fail:
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=trailing_newline
# Restore newline:
git diff -U0
...
💬 optout21 commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2536712096)
Yes, a similar check could be done in the beginning, when `tip` is retrieved. But prob. outside of the scope of this PR.
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2536712096)
Yes, a similar check could be done in the beginning, when `tip` is retrieved. But prob. outside of the scope of this PR.