💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064960)
We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn't work. I considered putting it into an existing functional test but wouldn't know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064960)
We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn't work. I considered putting it into an existing functional test but wouldn't know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065138)
Passing `target_weight` with an absolute `fee` should still work fine. There is arguably less reason now to do so as passing a `fee_rate` is likely more convenient for most use-cases, but I don't think we have to disallow padding txs with an absolute fee.
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065138)
Passing `target_weight` with an absolute `fee` should still work fine. There is arguably less reason now to do so as passing a `fee_rate` is likely more convenient for most use-cases, but I don't think we have to disallow padding txs with an absolute fee.
💬 theStack commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065536)
The actual weight can be larger by at most 3 WUs, [see comment above](https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751).
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616065536)
The actual weight can be larger by at most 3 WUs, [see comment above](https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1616064751).
✅ kosuodhmwa closed an issue: ""bitcoin-cli" does not exist, while "bitcoind" does in ~/bitcoin/src folder"
(https://github.com/bitcoin/bitcoin/issues/30180)
(https://github.com/bitcoin/bitcoin/issues/30180)
💬 kosuodhmwa commented on issue ""bitcoin-cli" does not exist, while "bitcoind" does in ~/bitcoin/src folder":
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133532648)
solved, was a mistake of me. thank you anyway :-)
(https://github.com/bitcoin/bitcoin/issues/30180#issuecomment-2133532648)
solved, was a mistake of me. thank you anyway :-)
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133774404)
@hebasto I do prefer to stay compatible with the `std::deque` interface, even if just for matching behavior users would expect.
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2133774404)
@hebasto I do prefer to stay compatible with the `std::deque` interface, even if just for matching behavior users would expect.
👍 itornaza approved a pull request: "consensus: fix `OP_1NEGATE` handling in `CScriptOp`"
(https://github.com/bitcoin/bitcoin/pull/29589#pullrequestreview-2081224349)
tested ACK 0abf50a71b0439ee7f4e94c548b29d726c58477a
Examined all changes from 3fb1b7043a7150f70a4cf1e172968e32c056dc79 up to this commit using `git difftool 3fb1b70 0abf50a` on the changed files. Did a code review on the changes that touches consensus code in`src/script/script.h` as well as the associated test file `test/functional/test_framework/script.py`.
The changes in the python test functions `encode_op_n` and `decode_op_n` are now one to one compatible with the consensus`EncodeOP_N
...
(https://github.com/bitcoin/bitcoin/pull/29589#pullrequestreview-2081224349)
tested ACK 0abf50a71b0439ee7f4e94c548b29d726c58477a
Examined all changes from 3fb1b7043a7150f70a4cf1e172968e32c056dc79 up to this commit using `git difftool 3fb1b70 0abf50a` on the changed files. Did a code review on the changes that touches consensus code in`src/script/script.h` as well as the associated test file `test/functional/test_framework/script.py`.
The changes in the python test functions `encode_op_n` and `decode_op_n` are now one to one compatible with the consensus`EncodeOP_N
...
💬 itornaza commented on pull request "consensus: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1616232765)
Non-blocking comment: Maybe move the `assert` line at the beginning of this member function so it is even more similar to its `EncodeOP_N` counterpart.
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1616232765)
Non-blocking comment: Maybe move the `assert` line at the beginning of this member function so it is even more similar to its `EncodeOP_N` counterpart.
💬 itornaza commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2133824938)
trACK 3d4dda0b26d3e003d282be1e30e64055750a792e
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2133824938)
trACK 3d4dda0b26d3e003d282be1e30e64055750a792e
💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2133888537)
cc: @achow101
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2133888537)
cc: @achow101
🤔 BrandonOdiwuor reviewed a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2081391076)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2081391076)
Concept ACK
👋 0xBEEFCAF3's pull request is ready for review: "Reenable OP_CAT"
(https://github.com/bitcoin/bitcoin/pull/29247)
(https://github.com/bitcoin/bitcoin/pull/29247)
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2134106601)
Here is a BIP PR for Testnet 4: https://github.com/bitcoin/bips/pull/1601
I think the written specification needs to be a BIP to be considered meaningful in the long run. If I just put it in a gist or something like that it depends on me alone to make changes should they become necessary for example. I would rather have it be managed by the community if the written specification is what people turn to.
The PR still might get changed obviously but I will update the BIP PR accordingly.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2134106601)
Here is a BIP PR for Testnet 4: https://github.com/bitcoin/bips/pull/1601
I think the written specification needs to be a BIP to be considered meaningful in the long run. If I just put it in a gist or something like that it depends on me alone to make changes should they become necessary for example. I would rather have it be managed by the community if the written specification is what people turn to.
The PR still might get changed obviously but I will update the BIP PR accordingly.
💬 cryptoquick commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2134338774)
Tested ACK
Works great against the Mutinynet node with the provided configuration, and the approach seems appropriate
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2134338774)
Tested ACK
Works great against the Mutinynet node with the provided configuration, and the approach seems appropriate
💬 Sjors commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2134456888)
@BenWestgate thanks for the analysis. If we want to add a recommendation to the help text, I suggest we do that in another PR though. It's probably going to take multiple people doing benchmark on wildly different machines.
Checking against RAM can also be done in a followup.
I rebased and changed the .md file name.
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2134456888)
@BenWestgate thanks for the analysis. If we want to add a recommendation to the help text, I suggest we do that in another PR though. It's probably going to take multiple people doing benchmark on wildly different machines.
Checking against RAM can also be done in a followup.
I rebased and changed the .md file name.
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616695926)
done.
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616695926)
done.
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616701338)
> I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).
yes, that can be done since the last bit is being sent from `MainThread` anyways.
before:
1. send both 4 bytes network magic (first 4 bytes of ellswift) - `NetworkThread`
2. remainin
...
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616701338)
> I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).
yes, that can be done since the last bit is being sent from `MainThread` anyways.
before:
1. send both 4 bytes network magic (first 4 bytes of ellswift) - `NetworkThread`
2. remainin
...
💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2134500918)
thank you for the helpful reviews @sr-gi!
I've updated the PR:
- `EARLY_KEY_RESPONSE` test doesn't have `magic_sent` now and the `initial_v2_handshake()` function is cleaner
- `EARLY_KEY_RESPONSE` test does sending of 4 bytes ellswift (which match network magic) + remaining ellswift and garbage bytes on `MainThread` - both are done manually now
- Added v2 handshake timeout log in 7d07daa
> What is weird is that the test is expecting a specific log message, which seems to also mat
...
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2134500918)
thank you for the helpful reviews @sr-gi!
I've updated the PR:
- `EARLY_KEY_RESPONSE` test doesn't have `magic_sent` now and the `initial_v2_handshake()` function is cleaner
- `EARLY_KEY_RESPONSE` test does sending of 4 bytes ellswift (which match network magic) + remaining ellswift and garbage bytes on `MainThread` - both are done manually now
- Added v2 handshake timeout log in 7d07daa
> What is weird is that the test is expecting a specific log message, which seems to also mat
...
👍 alfonsoromanz approved a pull request: "test: Assumeutxo: snapshots with less work should not be loaded"
(https://github.com/bitcoin/bitcoin/pull/29428#pullrequestreview-2082042285)
Re ACK df6dc2aaaeffc664006b86ee8c8797dc484ec40e. Make is successful and the test passes.
<img width="500" alt="Screenshot 2024-05-28 at 09 55 15" src="https://github.com/bitcoin/bitcoin/assets/19962151/e3e974e8-a507-49c9-8d77-6457f6f95443">
<img width="500" alt="Screenshot 2024-05-28 at 09 58 39" src="https://github.com/bitcoin/bitcoin/assets/19962151/f2e020ed-efc6-4854-97b5-f919ac1e0cdf">
(https://github.com/bitcoin/bitcoin/pull/29428#pullrequestreview-2082042285)
Re ACK df6dc2aaaeffc664006b86ee8c8797dc484ec40e. Make is successful and the test passes.
<img width="500" alt="Screenshot 2024-05-28 at 09 55 15" src="https://github.com/bitcoin/bitcoin/assets/19962151/e3e974e8-a507-49c9-8d77-6457f6f95443">
<img width="500" alt="Screenshot 2024-05-28 at 09 58 39" src="https://github.com/bitcoin/bitcoin/assets/19962151/f2e020ed-efc6-4854-97b5-f919ac1e0cdf">
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2134614071)
> if you have more thoughts on it
Whenever I talk to people interested in using Stratum v2 I find it difficult to explain all the moving parts they need to install and configure.
Install:
* a separate Bitcoin Core branch (temporary, pending #29432 merge)
* a job declaration client
* a sv1-sv2 translator (temporary, pending sv2 firmware support)
Configure:
* point job declaration client to Bitcoin Core, to the pools job declaration server and to the pool itself
* point translator to
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2134614071)
> if you have more thoughts on it
Whenever I talk to people interested in using Stratum v2 I find it difficult to explain all the moving parts they need to install and configure.
Install:
* a separate Bitcoin Core branch (temporary, pending #29432 merge)
* a job declaration client
* a sv1-sv2 translator (temporary, pending sv2 firmware support)
Configure:
* point job declaration client to Bitcoin Core, to the pools job declaration server and to the pool itself
* point translator to
...